From 6d8b512cb88c0bd4cfef72900a895a4fbe3ff4cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Fri, 5 Feb 2021 11:00:29 +0100 Subject: [PATCH 1/3] fix(java): JsiiRuntime.ErrorStreamSink does not respond to being interrupted When the thread is interrupted, it should understand the VM is asking it to shut down, and stop polling the child process' STDERR for more data. Previously, the thread did not check Thread.interrupted(), and hence was ignoring requests to terminate, resulting in ugly errors during clean-up. This adds the necessary check for interrupt status, and stops the error sink as asked by the VM. Fixes #2533 --- .../src/main/java/software/amazon/jsii/JsiiRuntime.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java index b079d11281..1b5ee81b55 100644 --- a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java +++ b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java @@ -362,7 +362,8 @@ public void run() { final BufferedReader reader = new BufferedReader(inputStreamReader)) { String line; final ObjectMapper objectMapper = new ObjectMapper(); - while ((line = reader.readLine()) != null) { + // Thread.interrupted() signals we should exit (the VM is asking us to terminate) + while (!Thread.interrupted() && (line = reader.readLine()) != null) { try { final JsonNode tree = objectMapper.readTree(line); final ConsoleOutput consoleOutput = objectMapper.treeToValue(tree, ConsoleOutput.class); From 552f9ef01ea4edff6457ffc1f1717399b1ecf36d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Mon, 8 Feb 2021 11:59:28 +0100 Subject: [PATCH 2/3] use zt-exec --- packages/@jsii/java-runtime/pom.xml.t.js | 8 + .../software/amazon/jsii/JsiiRuntime.java | 169 +++++++++--------- 2 files changed, 88 insertions(+), 89 deletions(-) diff --git a/packages/@jsii/java-runtime/pom.xml.t.js b/packages/@jsii/java-runtime/pom.xml.t.js index ae0381d875..13b82e58c9 100644 --- a/packages/@jsii/java-runtime/pom.xml.t.js +++ b/packages/@jsii/java-runtime/pom.xml.t.js @@ -65,6 +65,7 @@ process.stdout.write(` [13.0.0,20.0-a0) [5.7.0,5.8-a0) [3.5.13,4.0-a0) + [1.12,2.0-a0) @@ -127,6 +128,13 @@ process.stdout.write(` javax.annotation-api \${javax-annotations.version} + + + + org.zeroturnaround + zt-exec + \${zt-exec.version} + diff --git a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java index 1b5ee81b55..e262c86fdc 100644 --- a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java +++ b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java @@ -2,10 +2,12 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.core.JsonParseException; -import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.jetbrains.annotations.Nullable; +import org.zeroturnaround.exec.ProcessExecutor; +import org.zeroturnaround.exec.StartedProcess; +import org.zeroturnaround.exec.stream.LogOutputStream; import software.amazon.jsii.api.Callback; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.JsonNodeFactory; @@ -13,7 +15,12 @@ import java.io.*; import java.lang.reflect.InvocationTargetException; +import java.nio.channels.Channels; +import java.nio.channels.Pipe; import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.concurrent.TimeUnit; import static software.amazon.jsii.JsiiVersion.JSII_RUNTIME_VERSION; @@ -41,7 +48,7 @@ public final class JsiiRuntime { /** * The child procesds. */ - private Process childProcess; + private StartedProcess childProcess; /** * Child's standard output. @@ -51,7 +58,7 @@ public final class JsiiRuntime { /** * Child's standard input. */ - private BufferedWriter stdin; + private Writer stdin; /** * Handler for synchronous callbacks. Must be set using setCallbackHandler. @@ -173,45 +180,53 @@ protected void finalize() throws Throwable { } synchronized void terminate() { - try { - // The jsii Kernel process exists after having received the "exit" message - if (stdin != null) { + // The jsii Kernel process exists after having received the "exit" message + if (stdin != null) { + try { stdin.write("{\"exit\":0}\n"); stdin.close(); + } catch (final IOException ioe) { + // Ignore - the stream might have already been closed, if the child exited already. + } finally { stdin = null; } + } - if (childProcess != null) { - // Wait for the child process to complete - try { - // Giving the process up to 5 seconds to clean up and exit - if (!childProcess.waitFor(5, TimeUnit.SECONDS)) { - // If it's still not done, forcibly terminate it at this point. - childProcess.destroy(); - } - } catch (final InterruptedException ie) { - throw new RuntimeException(ie); + if (childProcess != null) { + // Wait for the child process to complete + try { + // Giving the process up to 5 seconds to clean up and exit + if (!childProcess.getProcess().waitFor(5, TimeUnit.SECONDS)) { + // If it's still not done, forcibly terminate it at this point. + childProcess.getProcess().destroyForcibly(); } + } catch (final InterruptedException ie) { + throw new RuntimeException(ie); + } finally { childProcess = null; } + } - // Cleaning up stdout (ensuring buffers are flushed, etc...) - if (stdout != null) { + // Cleaning up stdout (ensuring buffers are flushed, etc...) + if (stdout != null) { + try { stdout.close(); + } catch (final IOException ioe) { + // Ignore - the stream might have already been closed. + } finally { stdout = null; } + } - // We shut down already, no need for the shutdown hook anymore - if (this.shutdownHook != null) { - try { - Runtime.getRuntime().removeShutdownHook(this.shutdownHook); - } catch (final IllegalStateException ise) { - // VM Shutdown is in progress, removal is now impossible (and unnecessary) - } + // We shut down already, no need for the shutdown hook anymore + if (this.shutdownHook != null) { + try { + Runtime.getRuntime().removeShutdownHook(this.shutdownHook); + } catch (final IllegalStateException ise) { + // VM Shutdown is in progress, removal is now impossible (and unnecessary) + } finally { this.shutdownHook = null; } - } catch (final IOException ioe) { - throw new UncheckedIOException(ioe); } } @@ -224,49 +239,44 @@ private synchronized void startRuntimeIfNeeded() { } // If JSII_DEBUG is set, enable traces. - String jsiiDebug = System.getenv("JSII_DEBUG"); - boolean traceEnabled = jsiiDebug != null + final String jsiiDebug = System.getenv("JSII_DEBUG"); + final boolean traceEnabled = jsiiDebug != null && !jsiiDebug.isEmpty() && !jsiiDebug.equalsIgnoreCase("false") && !jsiiDebug.equalsIgnoreCase("0"); // If JSII_RUNTIME is set, use it to find the jsii-server executable // otherwise, we default to "jsii-runtime" from PATH. - String jsiiRuntimeExecutable = System.getenv("JSII_RUNTIME"); - if (jsiiRuntimeExecutable == null) { - jsiiRuntimeExecutable = BundledRuntime.extract(getClass()); - } + final String jsiiRuntimeEnv = System.getenv("JSII_RUNTIME"); + final List jsiiRuntimeCommand = jsiiRuntimeEnv == null + ? Arrays.asList("node", BundledRuntime.extract(getClass())) + : Collections.singletonList(jsiiRuntimeEnv); if (traceEnabled) { - System.err.println("jsii-runtime: " + jsiiRuntimeExecutable); + System.err.println("jsii-runtime: " + String.join(" ", jsiiRuntimeCommand)); } - ProcessBuilder pb = new ProcessBuilder("node", jsiiRuntimeExecutable) - .redirectInput(ProcessBuilder.Redirect.PIPE) - .redirectOutput(ProcessBuilder.Redirect.PIPE) - .redirectError(ProcessBuilder.Redirect.PIPE); + try { + final Pipe stdin = Pipe.open(); + this.stdin = Channels.newWriter(stdin.sink(), StandardCharsets.UTF_8); - if (traceEnabled) { - pb.environment().put("JSII_DEBUG", "1"); - } + final Pipe stdout = Pipe.open(); + this.stdout = new BufferedReader(Channels.newReader(stdout.source(), StandardCharsets.UTF_8)); - pb.environment().put("JSII_AGENT", "Java/" + System.getProperty("java.version")); + final ProcessExecutor executor = new ProcessExecutor(jsiiRuntimeCommand) + .environment("JSII_AGENT", String.format("Java/%s", System.getProperty("java.version"))) + .environment("JSII_DEBUG", jsiiDebug) + .redirectInput(Channels.newInputStream(stdin.source())) + .redirectOutput(Channels.newOutputStream(stdout.sink())) + .redirectError(new ErrorStreamSink()); - try { - this.childProcess = pb.start(); - this.shutdownHook = new Thread(this::terminate, "Terminate jsii client"); - Runtime.getRuntime().addShutdownHook(this.shutdownHook); - } catch (IOException e) { - throw new JsiiException("Cannot find the 'jsii-runtime' executable (JSII_RUNTIME or PATH)", e); + this.childProcess = executor.start(); + } catch (final IOException ioe) { + throw new UncheckedIOException(ioe); } - OutputStreamWriter stdinStream = new OutputStreamWriter(this.childProcess.getOutputStream(), StandardCharsets.UTF_8); - InputStreamReader stdoutStream = new InputStreamReader(this.childProcess.getInputStream(), StandardCharsets.UTF_8); - - new ErrorStreamSink(this.childProcess.getErrorStream()).start(); - - this.stdout = new BufferedReader(stdoutStream); - this.stdin = new BufferedWriter(stdinStream); + this.shutdownHook = new Thread(this::terminate, "Terminate jsii client"); + Runtime.getRuntime().addShutdownHook(this.shutdownHook); handshake(); @@ -346,41 +356,22 @@ private static void notifyInspector(final JsonNode message, final MessageInspect inspector.inspect(message, type); } - private static final class ErrorStreamSink extends Thread { - private final InputStream inputStream; - - public ErrorStreamSink(final InputStream inputStream) { - super("JsiiRuntime.ErrorStreamSink"); - // This is a daemon thread, shouldn't keep the VM alive. - this.setDaemon(true); + private static final class ErrorStreamSink extends LogOutputStream { + private final ObjectMapper objectMapper = new ObjectMapper(); - this.inputStream = inputStream; - } - - public void run() { - try (final InputStreamReader inputStreamReader = new InputStreamReader(this.inputStream); - final BufferedReader reader = new BufferedReader(inputStreamReader)) { - String line; - final ObjectMapper objectMapper = new ObjectMapper(); - // Thread.interrupted() signals we should exit (the VM is asking us to terminate) - while (!Thread.interrupted() && (line = reader.readLine()) != null) { - try { - final JsonNode tree = objectMapper.readTree(line); - final ConsoleOutput consoleOutput = objectMapper.treeToValue(tree, ConsoleOutput.class); - if (consoleOutput.stderr != null) { - System.err.write(consoleOutput.stderr, 0, consoleOutput.stderr.length); - } - if (consoleOutput.stdout != null) { - System.out.write(consoleOutput.stdout, 0, consoleOutput.stdout.length); - } - } catch (final JsonParseException | JsonMappingException exception) { - // If not JSON, then this goes straight to stderr without touches... - System.err.println(line); - } + public void processLine(final String line) { + try { + final JsonNode tree = objectMapper.readTree(line); + final ConsoleOutput consoleOutput = objectMapper.treeToValue(tree, ConsoleOutput.class); + if (consoleOutput.stderr != null) { + System.err.write(consoleOutput.stderr, 0, consoleOutput.stderr.length); + } + if (consoleOutput.stdout != null) { + System.out.write(consoleOutput.stdout, 0, consoleOutput.stdout.length); } - } catch (final IOException error) { - System.err.printf("I/O Error reading jsii Kernel's STDERR: %s%n", error); - throw new UncheckedIOException(error); + } catch (final JsonProcessingException exception) { + // If not JSON, then this goes straight to stderr without touches... + System.err.println(line); } } } From 913070b37924cc34c9efe539f0565f597fc1c701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Mon, 8 Feb 2021 12:32:03 +0100 Subject: [PATCH 3/3] use existing API for Channels.newReader/newWriter --- .../src/main/java/software/amazon/jsii/JsiiRuntime.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java index e262c86fdc..3db9444423 100644 --- a/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java +++ b/packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java @@ -258,10 +258,10 @@ private synchronized void startRuntimeIfNeeded() { try { final Pipe stdin = Pipe.open(); - this.stdin = Channels.newWriter(stdin.sink(), StandardCharsets.UTF_8); + this.stdin = Channels.newWriter(stdin.sink(), StandardCharsets.UTF_8.newEncoder(), -1); final Pipe stdout = Pipe.open(); - this.stdout = new BufferedReader(Channels.newReader(stdout.source(), StandardCharsets.UTF_8)); + this.stdout = new BufferedReader(Channels.newReader(stdout.source(), StandardCharsets.UTF_8.newDecoder(), -1)); final ProcessExecutor executor = new ProcessExecutor(jsiiRuntimeCommand) .environment("JSII_AGENT", String.format("Java/%s", System.getProperty("java.version")))