feat(dotnet,java): kernel process inherits host's STDERR #2248
Conversation
Removed the code that surrounded handling of the `STDERR` stream from the Kernel process: there is no need to PIPE it, and simply inheriting it (i.e: subprocess STDERR is the same stream as the parent's) results in a more intuitive behavior, as STDERR output from child processes surfaces "immediately" even if the parent process somehow cannot forward it anymore (e.g: if it crashed or deadlocked). This also means the STDERR forwarding thread is no longer needed.
RomainMuller
added
language/dotnet
| @@ -38,8 +38,7 @@ public NodeProcess(IJsiiRuntimeProvider jsiiRuntimeProvider, ILoggerFactory logg | |||
| StandardInputEncoding = utf8, | |||
| RedirectStandardOutput = true, | |||
| StandardOutputEncoding = utf8, | |||
| RedirectStandardError = true, | |||
| StandardErrorEncoding = utf8 | |||
| UseShellExecute = false, | |||
There was a problem hiding this comment.
I think it's a good change but usually has implications on being able to locate executables. Have you tested this on windows?
There was a problem hiding this comment.
- According to .NET's own documentation,
UseShellExecute = falseis required if you set anyRedirect*totrue. - The default value of this property is
falseon.NET Core(buttrueon.NET Framework). This is only making it explicit
| void IDisposable.Dispose() | ||
| { | ||
| StandardInput.Dispose(); |
There was a problem hiding this comment.
Why are these not needed anymore ?
There was a problem hiding this comment.
StandardInput, StandardOutput and StandardError are actually directly obtained from properties of the Process instance... When we call Dispose() on the process, it naturally disposes of all resources it owns, including those streams.
Additionally, Dispose() might abruptly terminate the stream, possibly without having flushed some buffered data. Calling Close() instead tends to ensure the buffer is flushed out before the close happens, so the node process gets the signal we are trying to send.
| @@ -10,19 +9,14 @@ internal sealed class Runtime : IRuntime | |||
| public Runtime(INodeProcess nodeProcess) | |||
| { | |||
| _nodeProcess = nodeProcess ?? throw new ArgumentNullException(nameof(nodeProcess)); | |||
| if (Environment.GetEnvironmentVariable("JSII_DEBUG") != null) | |||
There was a problem hiding this comment.
What functionality do we lose here?
There was a problem hiding this comment.
Previously, STDERR from anywhere in node (i.e: console.error from user's apps) were only surfaced to the console of non-JS applications when JSII_DEBUG was set to true. This means the behavior of a program was different if you ran it "pure JavaScript" versus through a jsii binding. Hence I think this was actually a bug and not a feature.
RomainMuller
force-pushed
the
rmuller/stderr-inherit
branch
from
c0fe1ec to
0bae44e
Compare
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Merging (with squash)... |
Removed the code that surrounded handling of the
STDERRstream fromthe Kernel process: there is no need to PIPE it, and simply inheriting
it (i.e: subprocess
STDERRis the same stream as the parent's) resultsin a more intuitive behavior, as
STDERRoutput from child processessurfaces "immediately" even if the parent process somehow cannot
forward it anymore (e.g: if it crashed or deadlocked). This also means
the
STDERRforwarding thread is no longer needed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.