From 6ba920355eb3cb4d1cdbefaca29aa42605eca305 Mon Sep 17 00:00:00 2001 From: nathan Date: Wed, 28 Oct 2015 16:35:24 +0100 Subject: [PATCH] Fixed issues with race conditions and piping content to null. Also fixed linux shell execution. --- .../util/process/LauncherProcessBuilder.java | 1 + .../dorkbox/util/process/ProcessProxy.java | 30 ++- .../util/process/ShellProcessBuilder.java | 234 +++++++++++------- 3 files changed, 175 insertions(+), 90 deletions(-) diff --git a/Dorkbox-Util/src/dorkbox/util/process/LauncherProcessBuilder.java b/Dorkbox-Util/src/dorkbox/util/process/LauncherProcessBuilder.java index ae6be21..16aa6e2 100644 --- a/Dorkbox-Util/src/dorkbox/util/process/LauncherProcessBuilder.java +++ b/Dorkbox-Util/src/dorkbox/util/process/LauncherProcessBuilder.java @@ -92,6 +92,7 @@ class LauncherProcessBuilder extends ShellProcessBuilder { setExecutable("dorkbox"); } + createReadWriterThreads(); // save off the original arguments List origArguments = new ArrayList(this.arguments.size()); diff --git a/Dorkbox-Util/src/dorkbox/util/process/ProcessProxy.java b/Dorkbox-Util/src/dorkbox/util/process/ProcessProxy.java index c3a35b8..1ac316d 100644 --- a/Dorkbox-Util/src/dorkbox/util/process/ProcessProxy.java +++ b/Dorkbox-Util/src/dorkbox/util/process/ProcessProxy.java @@ -18,12 +18,14 @@ package dorkbox.util.process; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.util.concurrent.CountDownLatch; public class ProcessProxy extends Thread { private final InputStream is; private final OutputStream os; + private final CountDownLatch countDownLatch = new CountDownLatch(1); // when reading from the stdin and outputting to the process public @@ -37,6 +39,7 @@ class ProcessProxy extends Thread { public void close() { + this.interrupt(); try { if (os != null) { os.flush(); // this goes to the console, so we don't want to close it! @@ -46,10 +49,27 @@ class ProcessProxy extends Thread { } } + @Override + public synchronized + void start() { + super.start(); + + // now we have to for it to actually start up. The process can run & complete before this starts, resulting in no input/output + // captured + try { + countDownLatch.await(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + } + @Override public void run() { final OutputStream os = this.os; + final InputStream is = this.is; + + countDownLatch.countDown(); try { // this thread will read until there is no more data to read. (this is generally what you want) @@ -58,17 +78,15 @@ class ProcessProxy extends Thread { if (os == null) { // just read so it won't block. - while ((readInt = this.is.read()) != -1) { + while (is.read() != -1) { } } else { - while ((readInt = this.is.read()) != -1) { + while ((readInt = is.read()) != -1) { os.write(readInt); - // flush the output on new line. - if (readInt == '\n') { - os.flush(); - } + // always flush after a write + os.flush(); } } } catch (Exception ignore) { diff --git a/Dorkbox-Util/src/dorkbox/util/process/ShellProcessBuilder.java b/Dorkbox-Util/src/dorkbox/util/process/ShellProcessBuilder.java index 064b769..ca5da95 100644 --- a/Dorkbox-Util/src/dorkbox/util/process/ShellProcessBuilder.java +++ b/Dorkbox-Util/src/dorkbox/util/process/ShellProcessBuilder.java @@ -40,7 +40,7 @@ public class ShellProcessBuilder { private final PrintStream outputStream; - private final PrintStream errorStream; + private final PrintStream outputErrorStream; private final InputStream inputStream; protected List arguments = new ArrayList(); @@ -51,6 +51,10 @@ class ShellProcessBuilder { // true if we want to save off (usually for debugging) the initial output from this private boolean debugInfo = false; + private boolean createReadWriterThreads = false; + + private boolean isShell; + private String pipeToNullString = ""; /** * This will cause the spawned process to pipe it's output to null. @@ -74,7 +78,23 @@ class ShellProcessBuilder { ShellProcessBuilder(InputStream in, PrintStream out, PrintStream err) { this.inputStream = in; this.outputStream = out; - this.errorStream = err; + this.outputErrorStream = err; + } + + /** + * Creates extra reader/writer threads for the sub-process. This is useful depending on how the sub-process is designed to run. + *

+ * For a process you want interactive IO with, this is required. + *

+ * For a long-running sub-process, with no interactive IO, this is what you'd want. + *

+ * For a run-and-get-the-results process, this isn't recommended. + * + */ + public final + ShellProcessBuilder createReadWriterThreads() { + createReadWriterThreads = true; + return this; } /** @@ -126,155 +146,189 @@ class ShellProcessBuilder { return this; } + public + ShellProcessBuilder pipeOutputToNull() { + if (OS.isWindows()) { + // >NUL on windows + pipeToNullString = ">NUL"; + } + else { + // we will "pipe" it to /dev/null on *nix + pipeToNullString = ">/dev/null 2>&1"; + } + + return this; + } + public void start() { + List argumentsList = new ArrayList(); + // if no executable, then use the command shell if (this.executableName == null) { + isShell = true; + if (OS.isWindows()) { // windows this.executableName = "cmd"; - this.arguments.add(0, "/c"); + argumentsList.add(this.executableName); + argumentsList.add("/c"); } else { // *nix this.executableName = "/bin/bash"; + File file = new File(this.executableName); if (!file.canExecute()) { this.executableName = "/bin/sh"; } - this.arguments.add(0, "-c"); - } - } - else if (this.workingDirectory != null) { - if (!this.workingDirectory.endsWith("/") && !this.workingDirectory.endsWith("\\")) { - this.workingDirectory += File.separator; + + argumentsList.add(this.executableName); + argumentsList.add("-c"); } } + else { + // shell and working/exe directory are mutually exclusive - if (this.executableDirectory != null) { - if (!this.executableDirectory.endsWith("/") && !this.executableDirectory.endsWith("\\")) { - this.executableDirectory += File.separator; - } - - this.executableName = this.executableDirectory + this.executableName; - } - - List argumentsList = new ArrayList(); - argumentsList.add(this.executableName); - - for (String arg : this.arguments) { - if (arg.contains(" ")) { - // individual arguments MUST be in their own element in order to - // be processed properly (this is how it works on the command line!) - String[] split = arg.split(" "); - for (String s : split) { - argumentsList.add(s); + if (this.workingDirectory != null) { + if (!this.workingDirectory.endsWith(File.separator)) { + this.workingDirectory += File.separator; } } - else { + + if (this.executableDirectory != null) { + if (!this.executableDirectory.endsWith(File.separator)) { + this.executableDirectory += File.separator; + } + + argumentsList.add(0, this.executableDirectory + this.executableName); + } else { + argumentsList.add(this.executableName); + } + } + + + // if we don't want output... + boolean pipeToNull = !pipeToNullString.isEmpty(); + + if (isShell && !OS.isWindows()) { + // when a shell AND on *nix, we have to place ALL the args into a single "arg" that is passed in + final StringBuilder stringBuilder = new StringBuilder(1024); + + for (String arg : this.arguments) { + stringBuilder.append(arg).append(" "); + } + + if (!arguments.isEmpty()) { + if (pipeToNull) { + stringBuilder.append(pipeToNullString); + } + else { + // delete last " " + stringBuilder.delete(stringBuilder.length() - 1, stringBuilder.length()); + } + } + + argumentsList.add(stringBuilder.toString()); + + } else { + for (String arg : this.arguments) { argumentsList.add(arg); } + + if (pipeToNull) { + argumentsList.add(pipeToNullString); + } } - // if we don't want output... TODO: i think we want to "exec" (this calls exec -c, which calls our program) - // this code as well, since calling it directly won't work - boolean pipeToNull = this.errorStream == null || this.outputStream == null; - if (pipeToNull) { - if (OS.isWindows()) { - // >NUL on windows - argumentsList.add(">NUL"); - } - else { - // we will "pipe" it to /dev/null on *nix - argumentsList.add(">/dev/null 2>&1"); - } - } if (this.debugInfo) { - if (errorStream != null) { - this.errorStream.print("Executing: "); + if (outputErrorStream != null) { + this.outputErrorStream.print("Executing: "); } else { - System.err.println("Executing: "); + System.err.print("Executing: "); } + Iterator iterator = argumentsList.iterator(); while (iterator.hasNext()) { String s = iterator.next(); - if (errorStream != null) { - this.errorStream.print(s); + if (outputErrorStream != null) { + this.outputErrorStream.print(s); } else { System.err.print(s); } if (iterator.hasNext()) { - if (errorStream != null) { - this.errorStream.print(" "); + if (outputErrorStream != null) { + this.outputErrorStream.print(" "); } else { System.err.print(" "); } } } - if (errorStream != null) { - this.errorStream.print(OS.LINE_SEPARATOR); + if (outputErrorStream != null) { + this.outputErrorStream.print(OS.LINE_SEPARATOR); } else { System.err.print(OS.LINE_SEPARATOR); } } + + + ProcessBuilder processBuilder = new ProcessBuilder(argumentsList); if (this.workingDirectory != null) { processBuilder.directory(new File(this.workingDirectory)); } // combine these so output is properly piped to null. - if (pipeToNull) { + if (pipeToNull || this.outputErrorStream == null) { processBuilder.redirectErrorStream(true); } try { this.process = processBuilder.start(); } catch (Exception ex) { - if (errorStream != null) { - this.errorStream.println("There was a problem executing the program. Details:"); + if (outputErrorStream != null) { + this.outputErrorStream.println("There was a problem executing the program. Details:"); } else { System.err.println("There was a problem executing the program. Details:"); } - ex.printStackTrace(this.errorStream); + ex.printStackTrace(this.outputErrorStream); if (this.process != null) { try { this.process.destroy(); this.process = null; } catch (Exception e) { - if (errorStream != null) { - this.errorStream.println("Error destroying process:"); + if (outputErrorStream != null) { + this.outputErrorStream.println("Error destroying process:"); } else { System.err.println("Error destroying process:"); } - e.printStackTrace(this.errorStream); + e.printStackTrace(this.outputErrorStream); } } } if (this.process != null) { - ProcessProxy writeToProcess_input; - ProcessProxy readFromProcess_output; - ProcessProxy readFromProcess_error; + ProcessProxy writeToProcess_input = null; + ProcessProxy readFromProcess_output = null; + ProcessProxy readFromProcess_error = null; + if (this.outputErrorStream == null && this.outputStream == null) { + if (!pipeToNull) { + NullOutputStream nullOutputStream = new NullOutputStream(); - if (pipeToNull) { - NullOutputStream nullOutputStream = new NullOutputStream(); - - processBuilder.redirectErrorStream(true); - - // readers (read process -> write console) - // have to keep the output buffers from filling in the target process. - readFromProcess_output = new ProcessProxy("Process Reader: " + this.executableName, - this.process.getInputStream(), - nullOutputStream); - readFromProcess_error = null; + // readers (read process -> write console) + // have to keep the output buffers from filling in the target process. + readFromProcess_output = new ProcessProxy("Process Reader: " + this.executableName, + this.process.getInputStream(), + nullOutputStream); + } } // we want to pipe our input/output from process to ourselves else { @@ -287,14 +341,10 @@ class ShellProcessBuilder { this.process.getInputStream(), this.outputStream); - if (this.errorStream != this.outputStream) { + if (this.outputErrorStream != this.outputStream) { readFromProcess_error = new ProcessProxy("Process Reader: " + this.executableName, this.process.getErrorStream(), - this.errorStream); - } - else { - processBuilder.redirectErrorStream(true); - readFromProcess_error = null; + this.outputErrorStream); } } @@ -307,9 +357,6 @@ class ShellProcessBuilder { this.inputStream, this.process.getOutputStream()); } - else { - writeToProcess_input = null; - } // the process can be killed in two ways @@ -320,7 +367,10 @@ class ShellProcessBuilder { public void run() { if (ShellProcessBuilder.this.debugInfo) { - ShellProcessBuilder.this.errorStream.println("Terminating process: " + ShellProcessBuilder.this.executableName); + final PrintStream errorStream = ShellProcessBuilder.this.outputErrorStream; + if (errorStream != null) { + errorStream.println("Terminating process: " + ShellProcessBuilder.this.executableName); + } } ShellProcessBuilder.this.process.destroy(); } @@ -330,11 +380,27 @@ class ShellProcessBuilder { .addShutdownHook(hook); if (writeToProcess_input != null) { - writeToProcess_input.start(); + if (createReadWriterThreads) { + writeToProcess_input.start(); + } + else { + writeToProcess_input.run(); + } + } + + if (createReadWriterThreads) { + readFromProcess_output.start(); + } + else { + readFromProcess_output.run(); } - readFromProcess_output.start(); if (readFromProcess_error != null) { - readFromProcess_error.start(); + if (createReadWriterThreads) { + readFromProcess_error.start(); + } + else { + readFromProcess_error.run(); + } } try {