Skip to content

Commit f4e90c3

Browse files
JeremyTCDJeremyTCD
authored
Merge pull request #146 from aKzenT/fix145
Fix handshake with Node.js not completing when external systems interfere with Node.js's stdout stream
2 parents 0517756 + 94effad commit f4e90c3

File tree

9 files changed

+104
-85
lines changed

9 files changed

+104
-85
lines changed

‎Changelog.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,13 @@ 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.1...HEAD)
6+
## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/7.0.0-beta.0...HEAD)
7+
8+
## [7.0.0-beta.0](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.1...7.0.0-beta.0) - Aug 25, 2022
9+
### Changes
10+
- **Breaking**: `OutOfProcessNodeJSService.OnConnectionEstablishedMessageReceived` now takes a `System.Text.RegularExpressions.Match` argument instead of a `string`. ([#146](https://github.com/JeringTech/Javascript.NodeJS/pull/146))
11+
### Fixes
12+
- Fixed handshake with Node.js not completing when external systems interfere with Node.js's stdout stream. ([#146](https://github.com/JeringTech/Javascript.NodeJS/pull/146))
713

814
## [6.3.1](https://github.com/JeringTech/Javascript.NodeJS/compare/6.3.0...6.3.1) - May 10, 2022
915
### Fixes

‎ReadMe.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1769,6 +1769,7 @@ Contributions are welcome!
17691769
- [flcdrg](https://github.com/flcdrg)
17701770
- [samcic](https://github.com/samcic)
17711771
- [johnrom](https://github.com/johnrom)
1772+
- [aKzenT](https://github.com/aKzenT)
17721773

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

‎azure-pipelines.yml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,3 @@ jobs:
1414
outOfProcessBuildDependencies: ["nodejs"]
1515
codecovKey: "e5de9f48-fb06-43c6-8368-44de5cf7e5d4"
1616
cacheYarnPackages: true
17-
- template: templates/docs/main.yml@templates
18-
parameters:
19-
nugetRestorePats: "$(nugetRestorePats)"

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
<PrivateAssets>all</PrivateAssets>
4949
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
5050
</PackageReference>
51-
<PackageReference Include="Nullable" Version="1.3.0">
51+
<PackageReference Include="Nullable" Version="1.3.1">
5252
<PrivateAssets>all</PrivateAssets>
5353
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
5454
</PackageReference>

‎src/NodeJS/NodeJSServiceImplementations/OutOfProcess/Http/HttpNodeJSService.cs‎

Lines changed: 19 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Net.Http;
77
using System.Reflection;
88
using System.Text;
9+
using System.Text.RegularExpressions;
910
using System.Threading;
1011
using System.Threading.Tasks;
1112

@@ -17,6 +18,11 @@ namespace Jering.Javascript.NodeJS
1718
/// </summary>
1819
public class HttpNodeJSService : OutOfProcessNodeJSService
1920
{
21+
/// <summary>
22+
/// Regex to match message used to perform a handshake with the NodeJS process.
23+
/// </summary>
24+
private static readonly Regex _sharedConnectionEstablishedMessageRegex = new(@"\[Jering\.Javascript\.NodeJS: HttpVersion - (?<protocol>HTTP\/\d\.\d) Listening on IP - (?<ip>[^ ]+) Port - (?<port>\d+)\]", RegexOptions.Compiled, TimeSpan.FromSeconds(1));
25+
2026
internal const string HTTP11_SERVER_SCRIPT_NAME = "Http11Server.js";
2127
internal const string HTTP20_SERVER_SCRIPT_NAME = "Http20Server.js";
2228

@@ -33,6 +39,9 @@ public class HttpNodeJSService : OutOfProcessNodeJSService
3339
// want to use the most recent instance
3440
internal volatile Uri? _endpoint;
3541

42+
/// <inheritdoc />
43+
protected override Regex ConnectionEstablishedMessageRegex => _sharedConnectionEstablishedMessageRegex;
44+
3645
/// <summary>
3746
/// Creates an <see cref="HttpNodeJSService"/>.
3847
/// </summary>
@@ -172,42 +181,18 @@ public HttpNodeJSService(IOptions<OutOfProcessNodeJSServiceOptions> outOfProcess
172181
}
173182

174183
/// <inheritdoc />
175-
protected override void OnConnectionEstablishedMessageReceived(string connectionEstablishedMessage)
184+
protected override void OnConnectionEstablishedMessageReceived(Match connectionMessageMatch)
176185
{
177-
// Start after message start and "HttpVersion - HTTP/X.X Listening on IP - "
178-
int startIndex = CONNECTION_ESTABLISHED_MESSAGE_START.Length + 41;
179-
var stringBuilder = new StringBuilder("http://");
180-
181-
for (int i = startIndex; i < connectionEstablishedMessage.Length; i++)
182-
{
183-
char currentChar = connectionEstablishedMessage[i];
184-
185-
if (currentChar == ':')
186+
_endpoint = new UriBuilder
186187
{
187-
// ::1
188-
stringBuilder.Append("[::1]");
189-
i += 2;
190-
}
191-
else if (currentChar == ' ')
192-
{
193-
stringBuilder.Append(':');
194-
195-
// Skip over "Port - "
196-
i += 7;
197-
}
198-
else if (currentChar == ']')
199-
{
200-
_endpoint = new Uri(stringBuilder.ToString());
201-
_logger.LogInformation(string.Format(Strings.LogInformation_HttpEndpoint,
202-
connectionEstablishedMessage.Substring(41, 8), // Pluck out HTTP version
203-
_endpoint));
204-
return;
205-
}
206-
else
207-
{
208-
stringBuilder.Append(currentChar);
209-
}
210-
}
188+
Scheme = "http",
189+
Host = connectionMessageMatch.Groups["ip"].Value,
190+
Port = int.Parse(connectionMessageMatch.Groups["port"].Value),
191+
}.Uri;
192+
193+
_logger.LogInformation(string.Format(Strings.LogInformation_HttpEndpoint,
194+
connectionMessageMatch.Groups["protocol"].Value, // Pluck out HTTP version
195+
_endpoint));
211196
}
212197

213198
/// <inheritdoc />

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

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.IO;
77
using System.Linq;
88
using System.Reflection;
9+
using System.Text.RegularExpressions;
910
using System.Threading;
1011
using System.Threading.Tasks;
1112

@@ -21,11 +22,6 @@ namespace Jering.Javascript.NodeJS
2122
/// <seealso cref="INodeJSService" />
2223
public abstract class OutOfProcessNodeJSService : INodeJSService
2324
{
24-
/// <summary>
25-
/// Start of the message used to perform a handshake with the NodeJS process.
26-
/// </summary>
27-
protected internal const string CONNECTION_ESTABLISHED_MESSAGE_START = "[Jering.Javascript.NodeJS: ";
28-
2925
/// <summary>
3026
/// The logger for the NodeJS process's stdout and stderr streams as well as messages from <see cref="OutOfProcessNodeJSService"/> and its implementations.
3127
/// </summary>
@@ -57,6 +53,12 @@ public abstract class OutOfProcessNodeJSService : INodeJSService
5753
private volatile INodeJSProcess? _nodeJSProcess; // Volatile since it's used in a double checked lock (we check whether it's null)
5854
private IFileWatcher? _fileWatcher;
5955

56+
/// <summary>
57+
/// <para>This regex is used to determine successful initialization of the process.</para>
58+
/// <para>All match groups contained in the regex are passed as arguments to the <see cref="OnConnectionEstablishedMessageReceived"/> method.</para>
59+
/// </summary>
60+
protected abstract Regex ConnectionEstablishedMessageRegex { get; }
61+
6062
/// <summary>
6163
/// Creates an <see cref="OutOfProcessNodeJSService"/> instance.
6264
/// </summary>
@@ -117,8 +119,8 @@ protected OutOfProcessNodeJSService(INodeJSProcessFactory nodeProcessFactory,
117119
/// <para>The message can be used to complete the handshake with the
118120
/// NodeJS process, for example by delivering a port and an IP address to use in further communications.</para>
119121
/// </summary>
120-
/// <param name="connectionEstablishedMessage">The connection established message.</param>
121-
protected abstract void OnConnectionEstablishedMessageReceived(string connectionEstablishedMessage);
122+
/// <param name="connectionMessageMatch">The regex match that can be used to extract additional arguments to complete the handshake.</param>
123+
protected abstract void OnConnectionEstablishedMessageReceived(Match connectionMessageMatch);
122124

123125
/// <inheritdoc />
124126
public virtual async Task<T?> InvokeFromFileAsync<T>(string modulePath, string? exportName = null, object?[]? args = null, CancellationToken cancellationToken = default)
@@ -671,9 +673,9 @@ internal void OutputReceivedHandler(string message, EventWaitHandle waitHandle)
671673
//
672674
// Note that we should not get a connection message for any process other than the current _nodeJSProcess
673675
// because ConnectIfNotConnected is synchronous.
674-
if (_nodeJSProcess?.Connected == false && message.StartsWith(CONNECTION_ESTABLISHED_MESSAGE_START))
676+
if (_nodeJSProcess?.Connected == false && ConnectionEstablishedMessageRegex.Match(message) is { Success: true } match)
675677
{
676-
OnConnectionEstablishedMessageReceived(message);
678+
OnConnectionEstablishedMessageReceived(match);
677679

678680
if (_infoLoggingEnabled)
679681
{

‎src/NodeJS/packages.lock.json‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,9 @@
6969
},
7070
"Nullable": {
7171
"type": "Direct",
72-
"requested": "[1.3.0, )",
73-
"resolved": "1.3.0",
74-
"contentHash": "xHAviTdTY3n+t1nEPN4JPRQR5lI124qRKVw+U9H7dO5sDNPpzoWeo/MQy7dSUmv9eD3k/CJVKokz1tFK+JOzRw=="
72+
"requested": "[1.3.1, )",
73+
"resolved": "1.3.1",
74+
"contentHash": "Mk4ZVDfAORTjvckQprCSehi1XgOAAlk5ez06Va/acRYEloN9t6d6zpzJRn5MEq7+RnagyFIq9r+kbWzLGd+6QA=="
7575
},
7676
"System.Text.Encodings.Web": {
7777
"type": "Direct",
@@ -255,9 +255,9 @@
255255
},
256256
"Nullable": {
257257
"type": "Direct",
258-
"requested": "[1.3.0, )",
259-
"resolved": "1.3.0",
260-
"contentHash": "xHAviTdTY3n+t1nEPN4JPRQR5lI124qRKVw+U9H7dO5sDNPpzoWeo/MQy7dSUmv9eD3k/CJVKokz1tFK+JOzRw=="
258+
"requested": "[1.3.1, )",
259+
"resolved": "1.3.1",
260+
"contentHash": "Mk4ZVDfAORTjvckQprCSehi1XgOAAlk5ez06Va/acRYEloN9t6d6zpzJRn5MEq7+RnagyFIq9r+kbWzLGd+6QA=="
261261
},
262262
"System.Text.Encodings.Web": {
263263
"type": "Direct",
@@ -509,9 +509,9 @@
509509
},
510510
"Nullable": {
511511
"type": "Direct",
512-
"requested": "[1.3.0, )",
513-
"resolved": "1.3.0",
514-
"contentHash": "xHAviTdTY3n+t1nEPN4JPRQR5lI124qRKVw+U9H7dO5sDNPpzoWeo/MQy7dSUmv9eD3k/CJVKokz1tFK+JOzRw=="
512+
"requested": "[1.3.1, )",
513+
"resolved": "1.3.1",
514+
"contentHash": "Mk4ZVDfAORTjvckQprCSehi1XgOAAlk5ez06Va/acRYEloN9t6d6zpzJRn5MEq7+RnagyFIq9r+kbWzLGd+6QA=="
515515
},
516516
"System.Text.Encodings.Web": {
517517
"type": "Direct",
@@ -750,9 +750,9 @@
750750
},
751751
"Nullable": {
752752
"type": "Direct",
753-
"requested": "[1.3.0, )",
754-
"resolved": "1.3.0",
755-
"contentHash": "xHAviTdTY3n+t1nEPN4JPRQR5lI124qRKVw+U9H7dO5sDNPpzoWeo/MQy7dSUmv9eD3k/CJVKokz1tFK+JOzRw=="
753+
"requested": "[1.3.1, )",
754+
"resolved": "1.3.1",
755+
"contentHash": "Mk4ZVDfAORTjvckQprCSehi1XgOAAlk5ez06Va/acRYEloN9t6d6zpzJRn5MEq7+RnagyFIq9r+kbWzLGd+6QA=="
756756
},
757757
"System.Text.Encodings.Web": {
758758
"type": "Direct",
@@ -918,9 +918,9 @@
918918
},
919919
"Nullable": {
920920
"type": "Direct",
921-
"requested": "[1.3.0, )",
922-
"resolved": "1.3.0",
923-
"contentHash": "xHAviTdTY3n+t1nEPN4JPRQR5lI124qRKVw+U9H7dO5sDNPpzoWeo/MQy7dSUmv9eD3k/CJVKokz1tFK+JOzRw=="
921+
"requested": "[1.3.1, )",
922+
"resolved": "1.3.1",
923+
"contentHash": "Mk4ZVDfAORTjvckQprCSehi1XgOAAlk5ez06Va/acRYEloN9t6d6zpzJRn5MEq7+RnagyFIq9r+kbWzLGd+6QA=="
924924
},
925925
"System.Text.Encodings.Web": {
926926
"type": "Direct",

‎test/NodeJS/HttpNodeJSServiceUnitTests.cs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using System.Net;
99
using System.Net.Http;
1010
using System.Text;
11+
using System.Text.RegularExpressions;
1112
using System.Threading;
1213
using System.Threading.Tasks;
1314
using Xunit;
@@ -207,27 +208,28 @@ public async Task TryInvokeAsync_ThrowsInvocationExceptionIfHttpResponseHasAnUne
207208

208209
[Theory]
209210
[MemberData(nameof(OnConnectionEstablishedMessageReceived_ExtractsEndPoint_Data))]
210-
public void OnConnectionEstablishedMessageReceived_ExtractsEndPoint(string dummyIP, string dummyPort, string expectedResult)
211+
public void OnConnectionEstablishedMessageReceived_ExtractsEndPoint(string dummyHttpVersion, string dummyIP, string dummyPort, string expectedResult)
211212
{
212213
// Arrange
213214
var loggerStringBuilder = new StringBuilder();
214-
string dummyConnectionEstablishedMessage = $"[Jering.Javascript.NodeJS: HttpVersion - HTTP/1.1 Listening on IP - {dummyIP} Port - {dummyPort}]";
215+
string dummyConnectionEstablishedMessage = $"[Jering.Javascript.NodeJS: HttpVersion - {dummyHttpVersion} Listening on IP - {dummyIP} Port - {dummyPort}]";
215216
ExposedHttpNodeJSService testSubject = CreateHttpNodeJSService(loggerStringBuilder: loggerStringBuilder);
216217

217218
// Act
218-
testSubject.ExposedOnConnectionEstablishedMessageReceived(dummyConnectionEstablishedMessage);
219+
testSubject.ExposedOnConnectionEstablishedMessageReceived(testSubject.ExposedConnectionEstablishedMessageRegex.Match(dummyConnectionEstablishedMessage));
219220

220221
// Assert
221222
Assert.Equal(expectedResult, testSubject._endpoint?.AbsoluteUri);
222-
Assert.Contains(string.Format(Strings.LogInformation_HttpEndpoint, "HTTP/1.1", expectedResult), loggerStringBuilder.ToString());
223+
Assert.Contains(string.Format(Strings.LogInformation_HttpEndpoint, dummyHttpVersion, expectedResult), loggerStringBuilder.ToString());
223224
}
224225

225226
public static IEnumerable<object[]> OnConnectionEstablishedMessageReceived_ExtractsEndPoint_Data()
226227
{
227228
return new object[][]
228229
{
229-
new object[]{"127.0.0.1", "12345", "http://127.0.0.1:12345/"}, // IPv4, arbitrary port
230-
new object[]{"::1", "543", "http://[::1]:543/"} // IPv6, arbitrary port
230+
new object[]{ "HTTP/1.1", "127.0.0.1", "12345", "http://127.0.0.1:12345/"}, // Http 1.1, IPv4, arbitrary port
231+
new object[]{ "HTTP/1.1", "::1", "543", "http://[::1]:543/"}, // Http 1.1, IPv6, arbitrary port
232+
new object[]{ "HTTP/2.0", "127.0.0.1", "12345", "http://127.0.0.1:12345/"} // Http 2.0, IPv4, arbitrary port
231233
};
232234
}
233235

@@ -326,14 +328,16 @@ public ExposedHttpNodeJSService(IOptions<OutOfProcessNodeJSServiceOptions> outOf
326328
{
327329
}
328330

331+
public Regex ExposedConnectionEstablishedMessageRegex => ConnectionEstablishedMessageRegex;
332+
329333
public Task<(bool, T?)> ExposedTryInvokeAsync<T>(InvocationRequest invocationRequest, CancellationToken cancellationToken)
330334
{
331335
return TryInvokeAsync<T>(invocationRequest, cancellationToken);
332336
}
333337

334-
public void ExposedOnConnectionEstablishedMessageReceived(string connectionEstablishedMessage)
338+
public void ExposedOnConnectionEstablishedMessageReceived(System.Text.RegularExpressions.Match connectionEstablishedMessageMatch)
335339
{
336-
OnConnectionEstablishedMessageReceived(connectionEstablishedMessage);
340+
OnConnectionEstablishedMessageReceived(connectionEstablishedMessageMatch);
337341
}
338342
}
339343
}

0 commit comments

Comments
 (0)