Added check for NumRetries = 0

Changelog.md

@@ -3,7 +3,11 @@ 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.3.0...HEAD)
+## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.1...HEAD)
+
+## [6.3.1](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.0...6.3.1) - May 10, 2022
+### Fixes
+- Fixed infinite retries issue that occurs when `OutOfProcessNodeJSServiceOptions.NumProcessRetries` > 0 and `OutOfProcessNodeJSServiceOptions.NumProcessRetries` === 0. ([#135](https://github.com/JeringTech/Javascript.NodeJS/pull/135))
 
 ## [6.3.0](https://github.com/JeringTech/Javascript.NodeJS/compare/6.2.0...6.3.0) - Dec 27, 2021
 ### Additions

License.md

@@ -1,5 +1,5 @@
 # Licenses
-Jering.Javascript.NodeJS, copyright © 2018-2021 Jering. All rights reserved.
+Jering.Javascript.NodeJS, copyright © 2018-2022 Jering. All rights reserved.
 
 ## Source Code and Documentation Code-Examples
 Source code and documentation code-examples are licensed under the following license:

ReadMe.md

@@ -1325,20 +1325,22 @@ If you require retries for such streams, copy their contents to a `MemoryStream`
 
 Defaults to 1.  
 ##### OutOfProcessNodeJSServiceOptions.NumProcessRetries
-The number of new NodeJS processes created to retry an invocation.  
+The number of NodeJS processes created to retry an invocation.  
 ```csharp
 public int NumProcessRetries { get; set; }
 ```
 ###### Remarks
 A NodeJS process retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times. Once a process's retries are exhausted,
-if any process retries remain, the library creates a new process that then retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times.  
+if any retry-processes remain, the library creates a new process and retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times.  
 
 For example, consider the situation where `OutOfProcessNodeJSServiceOptions.NumRetries` and this value are both 1. The existing process first attempts the invocation.
 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.  
+attempts the invocation 3 times.  
 
 If this value is negative, the library creates new NodeJS processes indefinitely.  
 
+If this value is larger than 0 and `OutOfProcessNodeJSServiceOptions.NumRetries` is 0, the invocation is retried once in each new process.  
+
 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.
@@ -1766,6 +1768,7 @@ Contributions are welcome!
 - [blushingpenguin](https://github.com/blushingpenguin)
 - [flcdrg](https://github.com/flcdrg)
 - [samcic](https://github.com/samcic)
+- [johnrom](https://github.com/johnrom)
 
 ## About
 Follow [@JeringTech](https://twitter.com/JeringTech) for updates and more.

src/NodeJS/Jering.Javascript.NodeJS.csproj

@@ -7,7 +7,7 @@
         <Authors>JeremyTCD</Authors>
         <Title>Invoke Javascript in NodeJS, from C#</Title>
         <Description>Jering.Javascript.NodeJS enables you to invoke javascript in NodeJS, from C#. With this ability, you can use NodeJS javascript libraries and scripts from C# projects.</Description>
-        <Copyright>© 2018-2021 Jering. All rights reserved.</Copyright>
+        <Copyright>© 2018-2022 Jering. All rights reserved.</Copyright>
         <PackageProjectUrl>https://www.jering.tech/utilities/jering.javascript.nodejs/index</PackageProjectUrl>
         <RepositoryUrl>https://github.com/JeringTech/Javascript.NodeJS</RepositoryUrl>
         <PackageLicenseUrl>$(RepositoryUrl)/blob/master/License.md</PackageLicenseUrl>

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSService.cs

@@ -328,7 +328,7 @@ public virtual void MoveToNewProcess()
                     }
 
                     numProcessRetries = numProcessRetries > 0 ? numProcessRetries - 1 : numProcessRetries; // numProcessRetries can be negative (retry indefinitely)
-                    numRetries = _numRetries - 1;
+                    numRetries = _numRetries > 0 ? _numRetries - 1 : _numRetries;
 
                     MoveToNewProcess(false);
                 }

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSServiceOptions.cs

@@ -24,19 +24,20 @@ public class OutOfProcessNodeJSServiceOptions
         /// </remarks>
         public int NumRetries { get; set; } = 1;
 
-        /// <summary>The number of new NodeJS processes created to retry an invocation.</summary>
-        /// <remarks>
-        /// <para>A NodeJS process retries invocations <see cref="NumRetries"/> times. Once a process's retries are exhausted,
-        /// if any <b>process retries</b> remain, the library creates a new process that then retries invocations <see cref="NumRetries"/> times.</para>
-        /// <para>For example, consider the situation where <see cref="NumRetries"/> and this value are both 1. The existing process first attempts the invocation.
-        /// 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>
+         /// <summary>The number of NodeJS processes created to retry an invocation.</summary>
+         /// <remarks>
+         /// <para>A NodeJS process retries invocations <see cref="NumRetries"/> times. Once a process's retries are exhausted,
+         /// if any <b>retry-processes</b> remain, the library creates a new process and retries invocations <see cref="NumRetries"/> times.</para>
+         /// <para>For example, consider the situation where <see cref="NumRetries"/> and this value are both 1. The existing process first attempts the invocation.
+         /// 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
+         /// attempts the invocation 3 times.</para>
+         /// <para>If this value is negative, the library creates new NodeJS processes indefinitely.</para>
+         /// <para>If this value is larger than 0 and <see cref="NumRetries"/> is 0, the invocation is retried once in each new process.</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>

test/NodeJS/OutOfProcessNodeJSServiceUnitTests.cs

@@ -555,6 +555,109 @@ public async void TryInvokeCoreAsync_RetriesInvocationsThatTimeoutAndThrowsInvoc
             mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(2));
         }
 
+        [Fact]
+        public async void TryInvokeCoreAsync_WithRetriesAndNoProcessRetries_RetriesOnlyOnCurrentProcess()
+        {
+            // Arrange
+            var dummyException = new OperationCanceledException();
+            const int dummyTimeoutMS = 100;
+            const int dummyNumRetries = 2;
+            const int dummyNumProcessRetries = 0;
+            var dummyOptions = new OutOfProcessNodeJSServiceOptions { 
+                TimeoutMS = dummyTimeoutMS, 
+                NumRetries = dummyNumRetries, 
+                NumProcessRetries = dummyNumProcessRetries 
+            };
+            Mock<IOptions<OutOfProcessNodeJSServiceOptions>> mockOptionsAccessor = _mockRepository.Create<IOptions<OutOfProcessNodeJSServiceOptions>>();
+            mockOptionsAccessor.Setup(o => o.Value).Returns(dummyOptions);
+            var loggerStringBuilder = new StringBuilder();
+            var dummyInvocationRequest = new InvocationRequest(ModuleSourceType.String, "dummyModuleSource");
+            var dummyCancellationToken = new CancellationToken();
+            Mock<OutOfProcessNodeJSService> mockTestSubject = CreateMockOutOfProcessNodeJSService(optionsAccessor: mockOptionsAccessor.Object,
+                loggerStringBuilder: loggerStringBuilder);
+            mockTestSubject.CallBase = true;
+            mockTestSubject.Setup(t => t.ConnectIfNotConnected());
+            mockTestSubject.
+                Protected().
+                As<IOutOfProcessNodeJSServiceProtectedMembers>().
+                Setup(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken)).
+                ThrowsAsync(dummyException);
+            mockTestSubject.Setup(t => t.CreateCancellationToken(dummyCancellationToken)).Returns((dummyCancellationToken, null));
+
+            // Act and assert
+            InvocationException result = await Assert.
+                ThrowsAsync<InvocationException>(async () => await mockTestSubject.Object.TryInvokeCoreAsync<int>(dummyInvocationRequest, dummyCancellationToken).ConfigureAwait(false)).
+                ConfigureAwait(false);
+            _mockRepository.VerifyAll();
+            // Verify log
+            string resultLog = loggerStringBuilder.ToString();
+            Assert.Equal(dummyNumRetries, Regex.Matches(resultLog, Strings.LogWarning_InvocationAttemptFailed.Substring(0, 30)).Count); // Logs after each retry
+            Assert.Empty(Regex.Matches(resultLog, Strings.LogWarning_RetriesInExistingProcessExhausted.Substring(0, 30))); // Logs before each process swap
+            // Verify calls
+            mockTestSubject.
+                Protected().
+                As<IOutOfProcessNodeJSServiceProtectedMembers>().
+                Verify(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken), Times.Exactly(dummyNumRetries + 1));
+            Assert.Equal(string.Format(Strings.InvocationException_OutOfProcessNodeJSService_InvocationTimedOut,
+                    dummyTimeoutMS,
+                    nameof(OutOfProcessNodeJSServiceOptions.TimeoutMS),
+                    nameof(OutOfProcessNodeJSServiceOptions)),
+                result.Message);
+            mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(0));
+        }
+
+        [Fact]
+        public async void TryInvokeCoreAsync_WithProcessRetriesAndNoRetries_RetriesOnlyOnNewProcess()
+        {
+            // Arrange
+            var dummyException = new OperationCanceledException();
+            const int dummyTimeoutMS = 100;
+            const int dummyNumRetries = 0;
+            const int dummyNumProcessRetries = 2;
+            var dummyOptions = new OutOfProcessNodeJSServiceOptions { 
+                TimeoutMS = dummyTimeoutMS, 
+                NumRetries = dummyNumRetries, 
+                NumProcessRetries = dummyNumProcessRetries 
+            };
+            Mock<IOptions<OutOfProcessNodeJSServiceOptions>> mockOptionsAccessor = _mockRepository.Create<IOptions<OutOfProcessNodeJSServiceOptions>>();
+            mockOptionsAccessor.Setup(o => o.Value).Returns(dummyOptions);
+            var loggerStringBuilder = new StringBuilder();
+            var dummyInvocationRequest = new InvocationRequest(ModuleSourceType.String, "dummyModuleSource");
+            var dummyCancellationToken = new CancellationToken();
+            Mock<OutOfProcessNodeJSService> mockTestSubject = CreateMockOutOfProcessNodeJSService(optionsAccessor: mockOptionsAccessor.Object,
+                loggerStringBuilder: loggerStringBuilder);
+            mockTestSubject.CallBase = true;
+            mockTestSubject.Setup(t => t.ConnectIfNotConnected());
+            mockTestSubject.
+                Protected().
+                As<IOutOfProcessNodeJSServiceProtectedMembers>().
+                Setup(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken)).
+                ThrowsAsync(dummyException);
+            mockTestSubject.Setup(t => t.CreateCancellationToken(dummyCancellationToken)).Returns((dummyCancellationToken, null));
+            mockTestSubject.Setup(t => t.MoveToNewProcess(false));
+
+            // Act and assert
+            InvocationException result = await Assert.
+                ThrowsAsync<InvocationException>(async () => await mockTestSubject.Object.TryInvokeCoreAsync<int>(dummyInvocationRequest, dummyCancellationToken).ConfigureAwait(false)).
+                ConfigureAwait(false);
+            _mockRepository.VerifyAll();
+            // Verify log
+            string resultLog = loggerStringBuilder.ToString();
+            Assert.Equal(dummyNumProcessRetries, Regex.Matches(resultLog, Strings.LogWarning_InvocationAttemptFailed.Substring(0, 30)).Count); // Logs after each retry
+            Assert.Equal(dummyNumProcessRetries, Regex.Matches(resultLog, Strings.LogWarning_RetriesInExistingProcessExhausted.Substring(0, 30)).Count); // Logs before each process swap
+            // Verify calls
+            mockTestSubject.
+                Protected().
+                As<IOutOfProcessNodeJSServiceProtectedMembers>().
+                Verify(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken), Times.Exactly(dummyNumProcessRetries + 1));
+            Assert.Equal(string.Format(Strings.InvocationException_OutOfProcessNodeJSService_InvocationTimedOut,
+                    dummyTimeoutMS,
+                    nameof(OutOfProcessNodeJSServiceOptions.TimeoutMS),
+                    nameof(OutOfProcessNodeJSServiceOptions)),
+                result.Message);
+            mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(dummyNumProcessRetries));
+        }
+
         [Fact]
         public async void TryInvokeCoreAsync_IfInvocationThrowsExceptionsOtherThanInvocationExceptionRetriesInTheSameProcessAndInANewProcessAndThrowsExceptionWhenNoRetriesRemain()
         {