Skip to content

Commit 0517756

Browse files
JeremyTCDJeremyTCD
authored
Merge pull request #135 from johnrom/johnrom/retries-0
Added check for NumRetries = 0
2 parents 4c44c07 + a854e90 commit 0517756

7 files changed

Lines changed: 131 additions & 20 deletions

File tree

‎Changelog.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@ This project uses [semantic versioning](http://semver.org/spec/v2.0.0.html). Ref
33
*[Semantic Versioning in Practice](https://www.jering.tech/articles/semantic-versioning-in-practice)*
44
for an overview of semantic versioning.
55

6-
## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.0...HEAD)
6+
## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.1...HEAD)
7+
8+
## [6.3.1](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.0...6.3.1) - May 10, 2022
9+
### Fixes
10+
- Fixed infinite retries issue that occurs when `OutOfProcessNodeJSServiceOptions.NumProcessRetries` > 0 and `OutOfProcessNodeJSServiceOptions.NumProcessRetries` === 0. ([#135](https://github.com/JeringTech/Javascript.NodeJS/pull/135))
711

812
## [6.3.0](https://github.com/JeringTech/Javascript.NodeJS/compare/6.2.0...6.3.0) - Dec 27, 2021
913
### Additions

‎License.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# Licenses
2-
Jering.Javascript.NodeJS, copyright © 2018-2021 Jering. All rights reserved.
2+
Jering.Javascript.NodeJS, copyright © 2018-2022 Jering. All rights reserved.
33

44
## Source Code and Documentation Code-Examples
55
Source code and documentation code-examples are licensed under the following license:

‎ReadMe.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,20 +1325,22 @@ If you require retries for such streams, copy their contents to a `MemoryStream`
13251325

13261326
Defaults to 1.
13271327
##### OutOfProcessNodeJSServiceOptions.NumProcessRetries
1328-
The number of new NodeJS processes created to retry an invocation.
1328+
The number of NodeJS processes created to retry an invocation.
13291329
```csharp
13301330
public int NumProcessRetries { get; set; }
13311331
```
13321332
###### Remarks
13331333
A NodeJS process retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times. Once a process's retries are exhausted,
1334-
if any process retries remain, the library creates a new process that then retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times.
1334+
if any retry-processes remain, the library creates a new process and retries invocations `OutOfProcessNodeJSServiceOptions.NumRetries` times.
13351335

13361336
For example, consider the situation where `OutOfProcessNodeJSServiceOptions.NumRetries` and this value are both 1. The existing process first attempts the invocation.
13371337
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
1338-
attempt the invocation 3 times.
1338+
attempts the invocation 3 times.
13391339

13401340
If this value is negative, the library creates new NodeJS processes indefinitely.
13411341

1342+
If this value is larger than 0 and `OutOfProcessNodeJSServiceOptions.NumRetries` is 0, the invocation is retried once in each new process.
1343+
13421344
By default, process retries are disabled for invocation failures caused by javascript errors. See `OutOfProcessNodeJSServiceOptions.EnableProcessRetriesForJavascriptErrors` for more information.
13431345

13441346
If the module source of an invocation is an unseekable stream, the invocation is not retried.
@@ -1766,6 +1768,7 @@ Contributions are welcome!
17661768
- [blushingpenguin](https://github.com/blushingpenguin)
17671769
- [flcdrg](https://github.com/flcdrg)
17681770
- [samcic](https://github.com/samcic)
1771+
- [johnrom](https://github.com/johnrom)
17691772

17701773
## About
17711774
Follow [@JeringTech](https://twitter.com/JeringTech) for updates and more.

‎src/NodeJS/Jering.Javascript.NodeJS.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<Authors>JeremyTCD</Authors>
88
<Title>Invoke Javascript in NodeJS, from C#</Title>
99
<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>
10-
<Copyright>© 2018-2021 Jering. All rights reserved.</Copyright>
10+
<Copyright>© 2018-2022 Jering. All rights reserved.</Copyright>
1111
<PackageProjectUrl>https://www.jering.tech/utilities/jering.javascript.nodejs/index</PackageProjectUrl>
1212
<RepositoryUrl>https://github.com/JeringTech/Javascript.NodeJS</RepositoryUrl>
1313
<PackageLicenseUrl>$(RepositoryUrl)/blob/master/License.md</PackageLicenseUrl>

‎src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ public virtual void MoveToNewProcess()
328328
}
329329

330330
numProcessRetries = numProcessRetries > 0 ? numProcessRetries - 1 : numProcessRetries; // numProcessRetries can be negative (retry indefinitely)
331-
numRetries = _numRetries - 1;
331+
numRetries = _numRetries > 0 ? _numRetries - 1 : _numRetries;
332332

333333
MoveToNewProcess(false);
334334
}

‎src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSServiceOptions.cs‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,20 @@ public class OutOfProcessNodeJSServiceOptions
2424
/// </remarks>
2525
public int NumRetries { get; set; } = 1;
2626

27-
/// <summary>The number of new NodeJS processes created to retry an invocation.</summary>
28-
/// <remarks>
29-
/// <para>A NodeJS process retries invocations <see cref="NumRetries"/> times. Once a process's retries are exhausted,
30-
/// if any <b>process retries</b> remain, the library creates a new process that then retries invocations <see cref="NumRetries"/> times.</para>
31-
/// <para>For example, consider the situation where <see cref="NumRetries"/> and this value are both 1. The existing process first attempts the invocation.
32-
/// 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
33-
/// attempt the invocation 3 times.</para>
34-
/// <para>If this value is negative, the library creates new NodeJS processes indefinitely.</para>
35-
/// <para>By default, process retries are disabled for invocation failures caused by javascript errors. See <see cref="EnableProcessRetriesForJavascriptErrors"/> for more information.</para>
36-
/// <para>If the module source of an invocation is an unseekable stream, the invocation is not retried.
37-
/// If you require retries for such streams, copy their contents to a <see cref="MemoryStream"/>.</para>
38-
/// <para>Defaults to 1.</para>
39-
/// </remarks>
27+
/// <summary>The number of NodeJS processes created to retry an invocation.</summary>
28+
/// <remarks>
29+
/// <para>A NodeJS process retries invocations <see cref="NumRetries"/> times. Once a process's retries are exhausted,
30+
/// if any <b>retry-processes</b> remain, the library creates a new process and retries invocations <see cref="NumRetries"/> times.</para>
31+
/// <para>For example, consider the situation where <see cref="NumRetries"/> and this value are both 1. The existing process first attempts the invocation.
32+
/// 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
33+
/// attempts the invocation 3 times.</para>
34+
/// <para>If this value is negative, the library creates new NodeJS processes indefinitely.</para>
35+
/// <para>If this value is larger than 0 and <see cref="NumRetries"/> is 0, the invocation is retried once in each new process.</para>
36+
/// <para>By default, process retries are disabled for invocation failures caused by javascript errors. See <see cref="EnableProcessRetriesForJavascriptErrors"/> for more information.</para>
37+
/// <para>If the module source of an invocation is an unseekable stream, the invocation is not retried.
38+
/// If you require retries for such streams, copy their contents to a <see cref="MemoryStream"/>.</para>
39+
/// <para>Defaults to 1.</para>
40+
/// </remarks>
4041
public int NumProcessRetries { get; set; } = 1;
4142

4243
/// <summary>Whether invocation failures caused by Javascript errors are retried in new processes.</summary>

‎test/NodeJS/OutOfProcessNodeJSServiceUnitTests.cs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,109 @@ public async void TryInvokeCoreAsync_RetriesInvocationsThatTimeoutAndThrowsInvoc
555555
mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(2));
556556
}
557557

558+
[Fact]
559+
public async void TryInvokeCoreAsync_WithRetriesAndNoProcessRetries_RetriesOnlyOnCurrentProcess()
560+
{
561+
// Arrange
562+
var dummyException = new OperationCanceledException();
563+
const int dummyTimeoutMS = 100;
564+
const int dummyNumRetries = 2;
565+
const int dummyNumProcessRetries = 0;
566+
var dummyOptions = new OutOfProcessNodeJSServiceOptions {
567+
TimeoutMS = dummyTimeoutMS,
568+
NumRetries = dummyNumRetries,
569+
NumProcessRetries = dummyNumProcessRetries
570+
};
571+
Mock<IOptions<OutOfProcessNodeJSServiceOptions>> mockOptionsAccessor = _mockRepository.Create<IOptions<OutOfProcessNodeJSServiceOptions>>();
572+
mockOptionsAccessor.Setup(o => o.Value).Returns(dummyOptions);
573+
var loggerStringBuilder = new StringBuilder();
574+
var dummyInvocationRequest = new InvocationRequest(ModuleSourceType.String, "dummyModuleSource");
575+
var dummyCancellationToken = new CancellationToken();
576+
Mock<OutOfProcessNodeJSService> mockTestSubject = CreateMockOutOfProcessNodeJSService(optionsAccessor: mockOptionsAccessor.Object,
577+
loggerStringBuilder: loggerStringBuilder);
578+
mockTestSubject.CallBase = true;
579+
mockTestSubject.Setup(t => t.ConnectIfNotConnected());
580+
mockTestSubject.
581+
Protected().
582+
As<IOutOfProcessNodeJSServiceProtectedMembers>().
583+
Setup(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken)).
584+
ThrowsAsync(dummyException);
585+
mockTestSubject.Setup(t => t.CreateCancellationToken(dummyCancellationToken)).Returns((dummyCancellationToken, null));
586+
587+
// Act and assert
588+
InvocationException result = await Assert.
589+
ThrowsAsync<InvocationException>(async () => await mockTestSubject.Object.TryInvokeCoreAsync<int>(dummyInvocationRequest, dummyCancellationToken).ConfigureAwait(false)).
590+
ConfigureAwait(false);
591+
_mockRepository.VerifyAll();
592+
// Verify log
593+
string resultLog = loggerStringBuilder.ToString();
594+
Assert.Equal(dummyNumRetries, Regex.Matches(resultLog, Strings.LogWarning_InvocationAttemptFailed.Substring(0, 30)).Count); // Logs after each retry
595+
Assert.Empty(Regex.Matches(resultLog, Strings.LogWarning_RetriesInExistingProcessExhausted.Substring(0, 30))); // Logs before each process swap
596+
// Verify calls
597+
mockTestSubject.
598+
Protected().
599+
As<IOutOfProcessNodeJSServiceProtectedMembers>().
600+
Verify(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken), Times.Exactly(dummyNumRetries + 1));
601+
Assert.Equal(string.Format(Strings.InvocationException_OutOfProcessNodeJSService_InvocationTimedOut,
602+
dummyTimeoutMS,
603+
nameof(OutOfProcessNodeJSServiceOptions.TimeoutMS),
604+
nameof(OutOfProcessNodeJSServiceOptions)),
605+
result.Message);
606+
mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(0));
607+
}
608+
609+
[Fact]
610+
public async void TryInvokeCoreAsync_WithProcessRetriesAndNoRetries_RetriesOnlyOnNewProcess()
611+
{
612+
// Arrange
613+
var dummyException = new OperationCanceledException();
614+
const int dummyTimeoutMS = 100;
615+
const int dummyNumRetries = 0;
616+
const int dummyNumProcessRetries = 2;
617+
var dummyOptions = new OutOfProcessNodeJSServiceOptions {
618+
TimeoutMS = dummyTimeoutMS,
619+
NumRetries = dummyNumRetries,
620+
NumProcessRetries = dummyNumProcessRetries
621+
};
622+
Mock<IOptions<OutOfProcessNodeJSServiceOptions>> mockOptionsAccessor = _mockRepository.Create<IOptions<OutOfProcessNodeJSServiceOptions>>();
623+
mockOptionsAccessor.Setup(o => o.Value).Returns(dummyOptions);
624+
var loggerStringBuilder = new StringBuilder();
625+
var dummyInvocationRequest = new InvocationRequest(ModuleSourceType.String, "dummyModuleSource");
626+
var dummyCancellationToken = new CancellationToken();
627+
Mock<OutOfProcessNodeJSService> mockTestSubject = CreateMockOutOfProcessNodeJSService(optionsAccessor: mockOptionsAccessor.Object,
628+
loggerStringBuilder: loggerStringBuilder);
629+
mockTestSubject.CallBase = true;
630+
mockTestSubject.Setup(t => t.ConnectIfNotConnected());
631+
mockTestSubject.
632+
Protected().
633+
As<IOutOfProcessNodeJSServiceProtectedMembers>().
634+
Setup(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken)).
635+
ThrowsAsync(dummyException);
636+
mockTestSubject.Setup(t => t.CreateCancellationToken(dummyCancellationToken)).Returns((dummyCancellationToken, null));
637+
mockTestSubject.Setup(t => t.MoveToNewProcess(false));
638+
639+
// Act and assert
640+
InvocationException result = await Assert.
641+
ThrowsAsync<InvocationException>(async () => await mockTestSubject.Object.TryInvokeCoreAsync<int>(dummyInvocationRequest, dummyCancellationToken).ConfigureAwait(false)).
642+
ConfigureAwait(false);
643+
_mockRepository.VerifyAll();
644+
// Verify log
645+
string resultLog = loggerStringBuilder.ToString();
646+
Assert.Equal(dummyNumProcessRetries, Regex.Matches(resultLog, Strings.LogWarning_InvocationAttemptFailed.Substring(0, 30)).Count); // Logs after each retry
647+
Assert.Equal(dummyNumProcessRetries, Regex.Matches(resultLog, Strings.LogWarning_RetriesInExistingProcessExhausted.Substring(0, 30)).Count); // Logs before each process swap
648+
// Verify calls
649+
mockTestSubject.
650+
Protected().
651+
As<IOutOfProcessNodeJSServiceProtectedMembers>().
652+
Verify(t => t.TryInvokeAsync<int>(dummyInvocationRequest, dummyCancellationToken), Times.Exactly(dummyNumProcessRetries + 1));
653+
Assert.Equal(string.Format(Strings.InvocationException_OutOfProcessNodeJSService_InvocationTimedOut,
654+
dummyTimeoutMS,
655+
nameof(OutOfProcessNodeJSServiceOptions.TimeoutMS),
656+
nameof(OutOfProcessNodeJSServiceOptions)),
657+
result.Message);
658+
mockTestSubject.Verify(t => t.MoveToNewProcess(false), times: Times.Exactly(dummyNumProcessRetries));
659+
}
660+
558661
[Fact]
559662
public async void TryInvokeCoreAsync_IfInvocationThrowsExceptionsOtherThanInvocationExceptionRetriesInTheSameProcessAndInANewProcessAndThrowsExceptionWhenNoRetriesRemain()
560663
{

0 commit comments

Comments
 (0)