Merge pull request #124 from JeringTech/additions_and_changes_to_retries

Additions and changes to retries

Author: JeremyTCD

Changelog.md

@@ -3,7 +3,15 @@ This project uses [semantic versioning](http://semver.org/spec/v2.0.0.html). Ref
 *[Semantic Versioning in Practice](https://www.jering.tech/articles/semantic-versioning-in-practice)*
 for an overview of semantic versioning.
 
-## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/6.1.0...HEAD)
+## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/6.2.0...HEAD)
+
+## [6.2.0](https://github.com/JeringTech/Javascript.NodeJS/compare/6.1.0...6.2.0) - Nov 26, 2021
+### Additions
+- Added `OutOfProcessNodeJSServiceOptions.EnableProcessRetriesForJavascriptErrors` option. Enables users to choose whether process retries occur for 
+  invocations that fail due to Javascript errors. ([#124](https://github.com/JeringTech/Javascript.NodeJS/pull/124)).
+### Fixes
+- Fixed infinite process retries bug. ([#124](https://github.com/JeringTech/Javascript.NodeJS/pull/124)).
+- Fixed missing log entry for last retry. ([#124](https://github.com/JeringTech/Javascript.NodeJS/pull/124)).
 
 ## [6.1.0](https://github.com/JeringTech/Javascript.NodeJS/compare/6.0.1...6.1.0) - Nov 4, 2021
 ### Additions

ReadMe.md

@@ -1338,10 +1338,23 @@ attempt the invocation 3 times.
 
 If this value is negative, the library creates new NodeJS processes indefinitely.  
 
+By default, process retries are disabled for invocation failures caused by javascript errors. See `OutOfProcessNodeJSServiceOptions.EnableProcessRetriesForJavascriptErrors` for more information.  
+
 If the module source of an invocation is an unseekable stream, the invocation is not retried.
 If you require retries for such streams, copy their contents to a `MemoryStream`.  
 
 Defaults to 1.  
+##### OutOfProcessNodeJSServiceOptions.EnableProcessRetriesForJavascriptErrors
+Whether invocation failures caused by Javascript errors are retried in new processes.  
+```csharp
+public bool EnableProcessRetriesForJavascriptErrors { get; set; }
+```
+###### Remarks
+Process retries were introduced to deal with process-level issues. For example, when a NodeJS process becomes unresponsive the only solution is to start a new process.  
+
+If this value is `true`, process retries also occur on Javascript errors. If it is `false`, they only occur for process-level issues.  
+
+Defaults to `false`.  
 ##### OutOfProcessNodeJSServiceOptions.NumConnectionRetries
 Number of times the library retries NodeJS connection attempts.  
 ```csharp

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSService.cs

@@ -47,6 +47,7 @@ public abstract class OutOfProcessNodeJSService : INodeJSService
         private readonly int _numRetries;
         private readonly int _numProcessRetries;
         private readonly int _numConnectionRetries;
+        private readonly bool _enableProcessRetriesForJavascriptErrors;
         private readonly int _timeoutMS;
         private readonly ConcurrentDictionary<Task, object?> _trackedInvokeTasks; // TODO use ConcurrentSet when it's available - https://github.com/dotnet/runtime/issues/16443
         private readonly CountdownEvent _invokeTaskCreationCountdown;
@@ -97,6 +98,7 @@ protected OutOfProcessNodeJSService(INodeJSProcessFactory nodeProcessFactory,
             _numProcessRetries = _options.NumProcessRetries;
             _numConnectionRetries = _options.NumConnectionRetries;
             _timeoutMS = _options.TimeoutMS;
+            _enableProcessRetriesForJavascriptErrors = _options.EnableProcessRetriesForJavascriptErrors;
 
             (_trackInvokeTasks, _trackedInvokeTasks, _invokeTaskCreationCountdown) = InitializeFileWatching();
         }
@@ -108,7 +110,7 @@ protected OutOfProcessNodeJSService(INodeJSProcessFactory nodeProcessFactory,
         /// <param name="invocationRequest">The invocation request to send to the NodeJS process.</param>
         /// <param name="cancellationToken">A <see cref="CancellationToken"/> that can be used to cancel the invocation.</param>
         /// <returns>The task object representing the asynchronous operation.</returns>
-        protected abstract Task<(bool, T?)> TryInvokeAsync<T>(InvocationRequest invocationRequest, CancellationToken cancellationToken); 
+        protected abstract Task<(bool, T?)> TryInvokeAsync<T>(InvocationRequest invocationRequest, CancellationToken cancellationToken);
 
         /// <summary>
         /// <para>This method is called when the connection established message from the NodeJS process is received.</para>
@@ -300,32 +302,39 @@ public virtual void MoveToNewProcess()
                     {
                         Logger.LogWarning(string.Format(Strings.LogWarning_InvocationAttemptFailed, numRetries < 0 ? "infinity" : numRetries.ToString(), exception.ToString()));
                     }
+
+                    if (numRetries == 0 && exception is InvocationException && !_enableProcessRetriesForJavascriptErrors)
+                    {
+                        // Don't retry in new process if exception is caused by JS error and process retries for JS errors is not enabled
+                        throw;
+                    }
+                }
+                catch (Exception exception) when (_warningLoggingEnabled) // numRetries == 0 && numProcessRetries == 0
+                {
+                    Logger.LogWarning(string.Format(Strings.LogWarning_InvocationAttemptFailed, numRetries < 0 ? "infinity" : numRetries.ToString(), exception.ToString()));
+                    throw;
                 }
                 finally
                 {
                     cancellationTokenSource?.Dispose();
                 }
 
-                if (numRetries == 0)
+                if (numRetries == 0) // If we get here, numProcessRetries != 0
                 {
-                    // If retries in the existing process have been exhausted but process retries remain,
-                    // move to new process and reset numRetries.
-                    if (numProcessRetries > 0)
+                    // If retries in the existing process have been exhausted but process retries remain, move to new process and reset numRetries.
+                    if (_warningLoggingEnabled)
                     {
-                        if (_warningLoggingEnabled)
-                        {
-                            Logger.LogWarning(string.Format(Strings.LogWarning_RetriesInExistingProcessExhausted, numProcessRetries < 0 ? "infinity" : numProcessRetries.ToString()));
-                        }
+                        Logger.LogWarning(string.Format(Strings.LogWarning_RetriesInExistingProcessExhausted, numProcessRetries < 0 ? "infinity" : numProcessRetries.ToString()));
+                    }
 
-                        numProcessRetries = numProcessRetries > 0 ? numProcessRetries - 1 : numProcessRetries;
-                        numRetries = _numRetries - 1;
+                    numProcessRetries = numProcessRetries > 0 ? numProcessRetries - 1 : numProcessRetries; // numProcessRetries can be negative (retry indefinitely)
+                    numRetries = _numRetries - 1;
 
-                        MoveToNewProcess(false);
-                    }
+                    MoveToNewProcess(false);
                 }
                 else
                 {
-                    numRetries = numRetries > 0 ? numRetries - 1 : numRetries;
+                    numRetries = numRetries > 0 ? numRetries - 1 : numRetries; // numRetries can be negative (retry indefinitely)
                 }
             }
         }

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSServiceOptions.cs

@@ -32,12 +32,21 @@ public class OutOfProcessNodeJSServiceOptions
         /// If it fails, it retries the invocation once. If it fails again, the library creates a new process that retries the invocation once. In total, the library
         /// attempt the invocation 3 times.</para>
         /// <para>If this value is negative, the library creates new NodeJS processes indefinitely.</para>
+        /// <para>By default, process retries are disabled for invocation failures caused by javascript errors. See <see cref="EnableProcessRetriesForJavascriptErrors"/> for more information.</para>
         /// <para>If the module source of an invocation is an unseekable stream, the invocation is not retried.
         /// If you require retries for such streams, copy their contents to a <see cref="MemoryStream"/>.</para>
         /// <para>Defaults to 1.</para>
         /// </remarks>
         public int NumProcessRetries { get; set; } = 1;
 
+        /// <summary>Whether invocation failures caused by Javascript errors are retried in new processes.</summary>
+        /// <remarks>
+        /// <para>Process retries were introduced to deal with process-level issues. For example, when a NodeJS process becomes unresponsive the only solution is to start a new process.</para>
+        /// <para>If this value is <c>true</c>, process retries also occur on Javascript errors. If it is <c>false</c>, they only occur for process-level issues.</para>
+        /// <para>Defaults to <c>false</c>.</para>
+        /// </remarks>
+        public bool EnableProcessRetriesForJavascriptErrors { get; set; } = false;
+
         /// <summary>Number of times the library retries NodeJS connection attempts.</summary>
         /// <remarks>
         /// <para>If this value is negative, connection attempts are retried indefinitely.</para>

test/NodeJS/HttpNodeJSServiceIntegrationTests.cs

@@ -1161,13 +1161,13 @@ public async void MoveToNewProcess_MovesToNewProcess()
         }
 
         [Fact(Timeout = TIMEOUT_MS)]
-        public async void NewProcessRetries_RetriesFailedInvocationInNewProcess()
+        public async void NewProcessRetries_RetriesInvocationThatFailedDueToJavascriptErrorsInNewProcessIfSuchRetriesAreEnabled()
         {
             // Arrange
             const string dummyErrorMessagePrefix = "Error in process with ID: ";
             string dummyErrorThrowingModule = $"module.exports = (callback) => {{throw new Error('{dummyErrorMessagePrefix}' + process.pid);}}";
             var resultStringBuilder = new StringBuilder();
-            HttpNodeJSService testSubject = CreateHttpNodeJSService(resultStringBuilder);
+            HttpNodeJSService testSubject = CreateHttpNodeJSService(resultStringBuilder, enableProcessRetriesForJavascriptErrors: true);
 
             // Act and assert
             InvocationException result = await Assert.ThrowsAsync<InvocationException>(() => testSubject.InvokeFromStringAsync(dummyErrorThrowingModule)).ConfigureAwait(false);
@@ -1185,6 +1185,29 @@ public async void NewProcessRetries_RetriesFailedInvocationInNewProcess()
             Assert.NotEqual(firstExceptionProcessID, thirdExceptionProcessID); // Invocation invoked in new process
         }
 
+        [Fact(Timeout = TIMEOUT_MS)]
+        public async void NewProcessRetries_DoesNotRetryInvocationThatFailedDueToJavascriptErrorsInNewProcessIfSuchRetriesAreDisabled()
+        {
+            // Arrange
+            const string dummyErrorMessagePrefix = "Error in process with ID: ";
+            string dummyErrorThrowingModule = $"module.exports = (callback) => {{throw new Error('{dummyErrorMessagePrefix}' + process.pid);}}";
+            var resultStringBuilder = new StringBuilder();
+            HttpNodeJSService testSubject = CreateHttpNodeJSService(resultStringBuilder, enableProcessRetriesForJavascriptErrors: false);
+
+            // Act and assert
+            InvocationException result = await Assert.ThrowsAsync<InvocationException>(() => testSubject.InvokeFromStringAsync(dummyErrorThrowingModule)).ConfigureAwait(false);
+            // Process ID of NodeJS process first two errors were thrown in
+            string resultLog = resultStringBuilder.ToString();
+            MatchCollection logMatches = Regex.Matches(resultLog, $"{typeof(InvocationException).FullName}: {dummyErrorMessagePrefix}(\\d+)");
+            string firstExceptionProcessID = logMatches[0].Groups[1].Captures[0].Value;
+            string secondExceptionProcessID = logMatches[1].Groups[1].Captures[0].Value;
+            Assert.Equal(firstExceptionProcessID, secondExceptionProcessID);
+            // Process ID of NodeJS process last error was thrown in
+            MatchCollection exceptionMessageMatches = Regex.Matches(result.Message, $"^{dummyErrorMessagePrefix}(\\d+)");
+            string thirdExceptionProcessID = exceptionMessageMatches[0].Groups[1].Captures[0].Value;
+            Assert.Equal(firstExceptionProcessID, thirdExceptionProcessID); // Invocation not invoked in new process
+        }
+
 #if NET5_0 || NETCOREAPP3_1
         [Theory]
         [MemberData(nameof(HttpVersion_IsConfigurable_Data))]
@@ -1221,7 +1244,8 @@ private HttpNodeJSService CreateHttpNodeJSService(StringBuilder? loggerStringBui
 #if NETCOREAPP3_1 || NET5_0
             Version? httpVersion = default,
 #endif
-            ServiceCollection? services = default)
+            ServiceCollection? services = default,
+            bool enableProcessRetriesForJavascriptErrors = false)
         {
             services ??= new ServiceCollection();
             services.AddNodeJS(); // Default INodeJSService is HttpNodeService
@@ -1256,6 +1280,8 @@ private HttpNodeJSService CreateHttpNodeJSService(StringBuilder? loggerStringBui
                 services.Configure<OutOfProcessNodeJSServiceOptions>(options => options.TimeoutMS = -1);
             }
 
+            services.Configure<OutOfProcessNodeJSServiceOptions>(options => options.EnableProcessRetriesForJavascriptErrors = enableProcessRetriesForJavascriptErrors);
+
             _serviceProvider = services.BuildServiceProvider();
 
             return (HttpNodeJSService)_serviceProvider.GetRequiredService<INodeJSService>();