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 1 commit
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 @@ -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 @@ -38,8 +38,7 @@ public NodeProcess(IJsiiRuntimeProvider jsiiRuntimeProvider, ILoggerFactory logg
StandardInputEncoding = utf8,
RedirectStandardOutput = true,
StandardOutputEncoding = utf8,
RedirectStandardError = true,
StandardErrorEncoding = utf8
UseShellExecute = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good change but usually has implications on being able to locate executables. Have you tested this on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • According to .NET's own documentation, UseShellExecute = false is required if you set any Redirect* to true.
  • The default value of this property is false on .NET Core (but true on .NET Framework). This is only making it explicit

}
};

Expand All @@ -58,18 +57,23 @@ public NodeProcess(IJsiiRuntimeProvider jsiiRuntimeProvider, ILoggerFactory logg
_process.Start();
}

~NodeProcess() {
((IDisposable)this).Dispose();
}

public TextWriter StandardInput => _process.StandardInput;

public TextReader StandardOutput => _process.StandardOutput;

public TextReader StandardError => _process.StandardError;

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();
StandardInput.Close();
_process.WaitForExit(5_000);
_process.Dispose();

// If Dispose() is called manually, there is no need to run the finalizer anymore, since
// this only calls Dispose(). So we inform the GC about this.
GC.SuppressFinalize(this);
}

/// <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());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import static software.amazon.jsii.JsiiVersion.JSII_RUNTIME_VERSION;
Expand All @@ -29,11 +30,6 @@ public final class JsiiRuntime {
*/
private static final String VERSION_BUILD_PART_REGEX = "\\+[a-z0-9]+$";

/**
* True to print server traces to STDERR.
*/
private static boolean traceEnabled = false;

/**
*
*/
Expand All @@ -49,11 +45,6 @@ public final class JsiiRuntime {
*/
private Process childProcess;

/**
* Child's standard error.
*/
private BufferedReader stderr;

/**
* Child's standard output.
*/
Expand Down Expand Up @@ -179,26 +170,23 @@ protected void finalize() throws Throwable {
}

public void terminate() throws InterruptedException, IOException {
if (stderr != null) {
stderr.close();
stderr = null;
}

if (stdout != null) {
stdout.close();
stdout = null;
}

if (stdin != null) {
if (stdin != null) {
stdin.close();
stdin = null;
}

if (childProcess != null) {
// Wait for the child process to complete
childProcess.waitFor();
if (!childProcess.waitFor(5, TimeUnit.SECONDS)) {
childProcess.destroy();
}
childProcess = null;
}

if (stdout != null) {
stdout.close();
stdout = null;
}
}

/**
Expand All @@ -211,12 +199,10 @@ private void startRuntimeIfNeeded() {

// If JSII_DEBUG is set, enable traces.
String jsiiDebug = System.getenv("JSII_DEBUG");
if (jsiiDebug != null
boolean traceEnabled = jsiiDebug != null
&& !jsiiDebug.isEmpty()
&& !jsiiDebug.equalsIgnoreCase("false")
&& !jsiiDebug.equalsIgnoreCase("0")) {
traceEnabled = true;
}
&& !jsiiDebug.equalsIgnoreCase("0");

// If JSII_RUNTIME is set, use it to find the jsii-server executable
// otherwise, we default to "jsii-runtime" from PATH.
Expand All @@ -229,7 +215,10 @@ private void startRuntimeIfNeeded() {
System.err.println("jsii-runtime: " + jsiiRuntimeExecutable);
}

ProcessBuilder pb = new ProcessBuilder("node", jsiiRuntimeExecutable);
ProcessBuilder pb = new ProcessBuilder("node", jsiiRuntimeExecutable)
.redirectInput(ProcessBuilder.Redirect.PIPE)
.redirectOutput(ProcessBuilder.Redirect.PIPE)
.redirectError(ProcessBuilder.Redirect.INHERIT);

if (traceEnabled) {
pb.environment().put("JSII_DEBUG", "1");
Expand All @@ -245,21 +234,13 @@ private void startRuntimeIfNeeded() {

OutputStreamWriter stdinStream = new OutputStreamWriter(this.childProcess.getOutputStream(), StandardCharsets.UTF_8);
InputStreamReader stdoutStream = new InputStreamReader(this.childProcess.getInputStream(), StandardCharsets.UTF_8);
InputStreamReader stderrStream = new InputStreamReader(this.childProcess.getErrorStream(), StandardCharsets.UTF_8);

this.stderr = new BufferedReader(stderrStream);
this.stdout = new BufferedReader(stdoutStream);
this.stdin = new BufferedWriter(stdinStream);

handshake();

this.client = new JsiiClient(this);

// if trace is enabled, start a thread that continuously reads from the child process's
// STDERR and prints to my STDERR.
if (traceEnabled) {
startPipeErrorStreamThread();
}
}

/**
Expand All @@ -286,8 +267,7 @@ JsonNode readNextResponse() {
try {
String responseLine = this.stdout.readLine();
if (responseLine == null) {
String error = this.stderr.lines().collect(Collectors.joining("\n\t"));
throw new JsiiException("Child process exited unexpectedly: " + error);
throw new JsiiException("Child process exited unexpectedly!");
}
final JsonNode response = JsiiObjectMapper.INSTANCE.readTree(responseLine);
JsiiRuntime.notifyInspector(response, MessageInspector.MessageType.Response);
Expand All @@ -297,28 +277,6 @@ JsonNode readNextResponse() {
}
}

/**
* Starts a thread that pipes STDERR from the child process to our STDERR.
*/
private void startPipeErrorStreamThread() {
Thread daemon = new Thread(() -> {
while (true) {
try {
String line = stderr.readLine();
System.err.println(line);
if (line == null) {
break;
}
} catch (IOException e) {
e.printStackTrace();
}
}
});

daemon.setDaemon(true);
daemon.start();
}

/**
* This will return the server process in case it is not already started.
* @return A {@link JsiiClient} connected to the server process.
Expand All @@ -331,13 +289,6 @@ public JsiiClient getClient() {
return this.client;
}

/**
* Prints jsii-server traces to STDERR.
*/
public static void enableTrace() {
traceEnabled = true;
}

/**
* Asserts that a peer runtimeVersion is compatible with this Java runtime version, which means
* they share the same version components, with the possible exception of the build number.
Expand Down