Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Jan 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ void IDisposable.Dispose()
JsiiTypeAttributeBase.Reset();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
{
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,5 @@ internal interface INodeProcess : IDisposable
TextWriter StandardInput { get; }

TextReader StandardOutput { get; }

TextReader StandardError { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these not needed anymore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Threading.Tasks;

namespace Amazon.JSII.Runtime.Services
{
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What functionality do we lose here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
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;
Expand All @@ -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());
}
}
}
}
Loading