Merge pull request #166 from aKzenT/fix_165

JeremyTCD · web-flow · commit 3977eaa6414c · 2023-04-18T20:24:45.000+08:00
Handle process outputs correctly. Fixes #165
diff --git a/Changelog.md b/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/7.0.0-beta.3...HEAD)
+## [Unreleased](https://github.com/JeringTech/Javascript.NodeJS/compare/7.0.0-beta.4...HEAD)
+
+## [7.0.0-beta.4](https://github.com/JeringTech/Javascript.NodeJS/compare/7.0.0-beta.3...7.0.0-beta.4) - Apr 18, 2023
+### Fixes
+- Handle process outputs correctly. ([#166](https://github.com/JeringTech/Javascript.NodeJS/pull/166)).
 
 ## [7.0.0-beta.3](https://github.com/JeringTech/Javascript.NodeJS/compare/7.0.0-beta.2...7.0.0-beta.3) - Feb 14, 2023
 ### Additions
diff --git a/src/NodeJS/NodeJSServiceImplementations/OutOfProcess/NodeJSProcess.cs b/src/NodeJS/NodeJSServiceImplementations/OutOfProcess/NodeJSProcess.cs
@@ -106,13 +106,14 @@ private void OutputThreadStart(object? arg)
             try
             {
                 string? data;
-                while ((data = streamReader.ReadLine()) != null)
+                do
                 {
+                    data = streamReader.ReadLine();
                     if (TryCreateMessage(_outputDataStringBuilder, data, out string? message))
                     {
                         _outputReceivedHandler!(message);
                     }
-                }
+                } while (data != null);
             }
             catch (IOException)
             {
@@ -131,13 +132,14 @@ private void ErrorThreadStart(object? arg)
             try
             {
                 string? data;
-                while ((data = streamReader.ReadLine()) != null)
+                do
                 {
+                    data = streamReader.ReadLine();
                     if (TryCreateMessage(_errorDataStringBuilder, data, out string? message))
                     {
                         _errorReceivedHandler!(message);
                     }
-                }
+                } while (data != null);
             }
             catch (IOException)
             {
@@ -387,13 +389,22 @@ protected virtual async ValueTask DisposeAsyncCore()
                     try
                     {
                         _process.Kill();
-                        await _process.WaitForExitAsync().ConfigureAwait(false);
                     }
                     catch
                     {
                         // Throws if process is already dead, note that process could die between HasExited check and Kill
                     }
                 }
+
+                // Wait for exit
+                //
+                // Even after HasExited is true, there may still be output to be read. See https://github.com/dotnet/runtime/blob/91b93eb22bc7d9029a38469e55aa72d52c087834/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs#L1475.
+                //
+                // Should not throw. Process has started so Process._haveProcessID and Process._haveProcessHandle are true. They are only set to false
+                // by Process.Close, which is only called by Process.Dispose, called below.
+                await _process.WaitForExitAsync().ConfigureAwait(false);
+
+                // Dispose
                 _process.Dispose();
 
                 _disposed = true;
@@ -445,13 +456,17 @@ protected virtual void Dispose(bool disposing)
                         try
                         {
                             _process?.Kill();
-                            _process?.WaitForExit(); // Blocks
                         }
                         catch
                         {
                             // Throws if process is already dead, note that process could die between HasExited check and Kill
                         }
                     }
+
+                    // Wait for exit (see DisposeAsync for why we call this outside of the HasExited == false block)
+                    _process?.WaitForExit(); // Blocks
+
+                    // Dispose
                     _process?.Dispose();
                 }
 

Original file line number	Diff line number	Diff line change
`@@ -106,13 +106,14 @@ private void OutputThreadStart(object? arg)`
`106`	`106`	`try`
`107`	`107`	`{`
`108`	`108`	`string? data;`
`109`		`- while ((data = streamReader.ReadLine()) != null)`
	`109`	`+ do`
`110`	`110`	`{`
	`111`	`+ data = streamReader.ReadLine();`
`111`	`112`	`if (TryCreateMessage(_outputDataStringBuilder, data, out string? message))`
`112`	`113`	`{`
`113`	`114`	`_outputReceivedHandler!(message);`
`114`	`115`	`}`
`115`		`- }`
	`116`	`+ } while (data != null);`
`116`	`117`	`}`
`117`	`118`	`catch (IOException)`
`118`	`119`	`{`
`@@ -131,13 +132,14 @@ private void ErrorThreadStart(object? arg)`
`131`	`132`	`try`
`132`	`133`	`{`
`133`	`134`	`string? data;`
`134`		`- while ((data = streamReader.ReadLine()) != null)`
	`135`	`+ do`
`135`	`136`	`{`
	`137`	`+ data = streamReader.ReadLine();`
`136`	`138`	`if (TryCreateMessage(_errorDataStringBuilder, data, out string? message))`
`137`	`139`	`{`
`138`	`140`	`_errorReceivedHandler!(message);`
`139`	`141`	`}`
`140`		`- }`
	`142`	`+ } while (data != null);`
`141`	`143`	`}`
`142`	`144`	`catch (IOException)`
`143`	`145`	`{`
`@@ -387,13 +389,22 @@ protected virtual async ValueTask DisposeAsyncCore()`
`387`	`389`	`try`
`388`	`390`	`{`
`389`	`391`	`_process.Kill();`
`390`		`- await _process.WaitForExitAsync().ConfigureAwait(false);`
`391`	`392`	`}`
`392`	`393`	`catch`
`393`	`394`	`{`
`394`	`395`	`// Throws if process is already dead, note that process could die between HasExited check and Kill`
`395`	`396`	`}`
`396`	`397`	`}`
	`398`	`+`
	`399`	`+ // Wait for exit`
	`400`	`+ //`
	`401`	`+ // Even after HasExited is true, there may still be output to be read. See https://github.com/dotnet/runtime/blob/91b93eb22bc7d9029a38469e55aa72d52c087834/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs#L1475.`
	`402`	`+ //`
	`403`	`+ // Should not throw. Process has started so Process._haveProcessID and Process._haveProcessHandle are true. They are only set to false`
	`404`	`+ // by Process.Close, which is only called by Process.Dispose, called below.`
	`405`	`+ await _process.WaitForExitAsync().ConfigureAwait(false);`
	`406`	`+`
	`407`	`+ // Dispose`
`397`	`408`	`_process.Dispose();`
`398`	`409`
`399`	`410`	`_disposed = true;`
`@@ -445,13 +456,17 @@ protected virtual void Dispose(bool disposing)`
`445`	`456`	`try`
`446`	`457`	`{`
`447`	`458`	`_process?.Kill();`
`448`		`- _process?.WaitForExit(); // Blocks`
`449`	`459`	`}`
`450`	`460`	`catch`
`451`	`461`	`{`
`452`	`462`	`// Throws if process is already dead, note that process could die between HasExited check and Kill`
`453`	`463`	`}`
`454`	`464`	`}`
	`465`	`+`
	`466`	`+ // Wait for exit (see DisposeAsync for why we call this outside of the HasExited == false block)`
	`467`	`+ _process?.WaitForExit(); // Blocks`
	`468`	`+`
	`469`	`+ // Dispose`
`455`	`470`	`_process?.Dispose();`
`456`	`471`	`}`
`457`	`472`