feat(dotnet,java): kernel process inherits host's STDERR (#2248)

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. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
aws · Jan 7, 2021 · 70ce153 · 70ce153
1 parent f1a06e5
commit 70ce153
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 121 deletions.
diff --git a/.../dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ServiceContainerFixture.cs b/.../dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ServiceContainerFixture.cs
@@ -28,4 +28,4 @@ void IDisposable.Dispose()
             JsiiTypeAttributeBase.Reset();
         }
     }
-}
+}
diff --git a/packages/@jsii/dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/XUnitLogger.cs b/packages/@jsii/dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/XUnitLogger.cs
@@ -4,41 +4,41 @@
 
 namespace Amazon.JSII.Runtime.IntegrationTests
 {
-    public sealed class XUnitLoggerFactory : ILoggerFactory
+    internal sealed class XUnitLoggerFactory : ILoggerFactory
     {
-        readonly ITestOutputHelper _output;
+        private readonly ITestOutputHelper _output;
 
         public XUnitLoggerFactory(ITestOutputHelper output)
         {
             _output = output ?? throw new ArgumentNullException(nameof(output));
         }
 
-        public void AddProvider(ILoggerProvider provider)
+        void ILoggerFactory.AddProvider(ILoggerProvider provider)
         {
         }
 
-        public ILogger CreateLogger(string categoryName)
+        ILogger ILoggerFactory.CreateLogger(string categoryName)
         {
             return new XUnitLogger(_output, categoryName);
         }
 
-        public void Dispose()
+        void IDisposable.Dispose()
         {
         }
     }
 
-    public sealed class XUnitLogger : ILogger, IDisposable
+    internal sealed class XUnitLogger : ILogger, IDisposable
     {
-        readonly ITestOutputHelper _output;
-        readonly string _categoryName;
+        private readonly ITestOutputHelper _output;
+        private readonly string _categoryName;
 
         public XUnitLogger(ITestOutputHelper output, string categoryName)
         {
             _output = output ?? throw new ArgumentNullException(nameof(output));
             _categoryName = categoryName ?? throw new ArgumentNullException(nameof(categoryName));
         }
 
-        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
+        void ILogger.Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
         {
             var str = state?.ToString() ?? "";
             // Only log lines starting with > or < (kernel traces)
@@ -48,17 +48,17 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
             }
         }
 
-        public IDisposable BeginScope<TState>(TState state)
+        IDisposable ILogger.BeginScope<TState>(TState state)
         {
             return this;
         }
 
-        public bool IsEnabled(LogLevel logLevel)
+        bool ILogger.IsEnabled(LogLevel logLevel)
         {
             return true;
         }
 
-        public void Dispose()
+        void IDisposable.Dispose()
         {
         }
     }

diff --git a/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime.UnitTests/Client/RuntimeTests.cs b/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime.UnitTests/Client/RuntimeTests.cs
@@ -11,17 +11,15 @@ public sealed class RuntimeTests
         private const string Prefix = "Runtime.";
 
         private INodeProcess _nodeProcessMock;
-        
+
         private IRuntime _sut;
 
         public RuntimeTests()
         {
             _nodeProcessMock = Substitute.For<INodeProcess>();
             var standardOutputMock = Substitute.For<TextReader>();
-            var standardErrorMock = Substitute.For<TextReader>();
 
             _nodeProcessMock.StandardOutput.Returns(standardOutputMock);
-            _nodeProcessMock.StandardError.Returns(standardErrorMock);
 
             _sut = new Services.Runtime(_nodeProcessMock);
         }
@@ -30,10 +28,9 @@ public RuntimeTests()
         public void ThrowsJsiiExceptionWhenResponseNotReceived()
         {
             _nodeProcessMock.StandardOutput.ReadLine().ReturnsNull();
-            _nodeProcessMock.StandardError.ReadToEnd().Returns("This is a test.");
 
             var ex = Assert.Throws<JsiiException>(() => _sut.ReadResponse());
-            Assert.Equal("Child process exited unexpectedly: This is a test.", ex.Message);
+            Assert.Equal("Child process exited unexpectedly!", ex.Message);
         }
     }
-}
+}
diff --git a/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/INodeProcess.cs b/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/INodeProcess.cs
@@ -8,7 +8,5 @@ internal interface INodeProcess : IDisposable
         TextWriter StandardInput { get; }
 
         TextReader StandardOutput { get; }
-
-        TextReader StandardError { get; }
     }
 }
diff --git a/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/NodeProcess.cs b/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/NodeProcess.cs
@@ -3,6 +3,7 @@
 using System.Globalization;
 using System.IO;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using System.Runtime.Versioning;
 using System.Text;
 using Microsoft.Extensions.Logging;
@@ -13,11 +14,14 @@ internal sealed class NodeProcess : INodeProcess
     {
         readonly Process _process;
         readonly ILogger _logger;
+
         private const string JsiiRuntime = "JSII_RUNTIME";
         private const string JsiiDebug = "JSII_DEBUG";
         private const string JsiiAgent = "JSII_AGENT";
         private const string JsiiAgentVersionString = "DotNet/{0}/{1}/{2}";
 
+        private bool Disposed = false;
+
         public NodeProcess(IJsiiRuntimeProvider jsiiRuntimeProvider, ILoggerFactory loggerFactory)
         {
             loggerFactory = loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory));
@@ -28,53 +32,79 @@ public NodeProcess(IJsiiRuntimeProvider jsiiRuntimeProvider, ILoggerFactory logg
                 runtimePath = jsiiRuntimeProvider.JsiiRuntimePath;
 
             var utf8 = new UTF8Encoding(false /* no BOM */);
-            _process = new Process
+            var startInfo = new ProcessStartInfo
             {
-                StartInfo = new ProcessStartInfo
-                {
-                    FileName = "node",
-                    Arguments = $"--max-old-space-size=4096 {runtimePath}",
-                    RedirectStandardInput = true,
-                    StandardInputEncoding = utf8,
-                    RedirectStandardOutput = true,
-                    StandardOutputEncoding = utf8,
-                    RedirectStandardError = true,
-                    StandardErrorEncoding = utf8
-                }
+                FileName = "node",
+                Arguments = $"--max-old-space-size=4096 {runtimePath}",
+                RedirectStandardInput = true,
+                StandardInputEncoding = utf8,
+                RedirectStandardOutput = true,
+                StandardOutputEncoding = utf8,
+                UseShellExecute = false,
             };
 
             var assemblyVersion = GetAssemblyFileVersion();
-            _process.StartInfo.EnvironmentVariables.Add(JsiiAgent,
+            startInfo.EnvironmentVariables.Add(JsiiAgent,
                 string.Format(CultureInfo.InvariantCulture, JsiiAgentVersionString, Environment.Version,
                     assemblyVersion.Item1, assemblyVersion.Item2));
 
             var debug = Environment.GetEnvironmentVariable(JsiiDebug);
-            if (!string.IsNullOrWhiteSpace(debug) && !_process.StartInfo.EnvironmentVariables.ContainsKey(JsiiDebug))
-                _process.StartInfo.EnvironmentVariables.Add(JsiiDebug, debug);
+            if (!string.IsNullOrWhiteSpace(debug) && !startInfo.EnvironmentVariables.ContainsKey(JsiiDebug))
+                startInfo.EnvironmentVariables.Add(JsiiDebug, debug);
 
             _logger.LogDebug("Starting jsii runtime...");
-            _logger.LogDebug($"{_process.StartInfo.FileName} {_process.StartInfo.Arguments}");
+            _logger.LogDebug($"{startInfo.FileName} {startInfo.Arguments}");
 
             // Registering shutdown hook to have JS process gracefully terminate.
             AppDomain.CurrentDomain.ProcessExit += (snd, evt) => {
                 ((IDisposable)this).Dispose();
             };
 
-            _process.Start();
-        }
+            _process = Process.Start(startInfo);
 
-        public TextWriter StandardInput => _process.StandardInput;
+            StandardInput = _process.StandardInput;
+            StandardOutput = _process.StandardOutput;
+        }
 
-        public TextReader StandardOutput => _process.StandardOutput;
+        public TextWriter StandardInput { get; }
 
-        public TextReader StandardError => _process.StandardError;
+        public TextReader StandardOutput { get; }
 
+        [MethodImpl(MethodImplOptions.Synchronized)]
         void IDisposable.Dispose()
         {
-            StandardInput.Dispose();
-            StandardOutput.Dispose();
-            StandardError.Dispose();
-            _process.Dispose();
+            if (Disposed) return;
+
+            Disposed = true;
+
+            // If the child process has already exited, we simply need to dispose of it
+            if (_process.HasExited)
+            {
+                _process.Dispose();
+                return;
+            }
+
+            // Closing the jsii Kernel's STDIN is how we instruct it to shut down
+            StandardInput.Close();
+
+            try
+            {
+                // Give the kernel 5 seconds to clean up after itself
+                if (!_process.WaitForExit(5_000)) {
+                    // Kill the child process if needed
+                    _process.Kill();
+                }
+            }
+            catch (InvalidOperationException)
+            {
+                // This means the process had already exited, because it was faster to clean up
+                // than we were to process it's termination. We still re-check if the process has
+                // exited and re-throw if not (meaning it was a different issue).
+                if (!_process.HasExited)
+                {
+                    throw;
+                }
+            }
         }
 
         /// <summary>

diff --git a/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/Runtime.cs b/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/Services/Runtime.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Threading.Tasks;
 
 namespace Amazon.JSII.Runtime.Services
 {
@@ -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)
-            {
-                Task.Run(() => RedirectStandardError());
-            }
         }
 
         public string ReadResponse()
         {
             var response = _nodeProcess.StandardOutput.ReadLine();
             if (string.IsNullOrEmpty(response))
             {
-                var errorMessage = _nodeProcess.StandardError.ReadToEnd();
-                throw new JsiiException("Child process exited unexpectedly: " + errorMessage);
+                throw new JsiiException("Child process exited unexpectedly!");
             }
 
             return response;
@@ -43,13 +37,5 @@ public void WriteRequest(string request)
             _nodeProcess.StandardInput.WriteLine(request);
             _nodeProcess.StandardInput.Flush();
         }
-
-        private void RedirectStandardError()
-        {
-            while (true)
-            {
-                Console.Error.WriteLine(_nodeProcess.StandardError.ReadLine());
-            }
-        }
     }
 }