[ https://issues.apache.org/jira/browse/EXEC-63?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13217797#comment-13217797 ]
Martin Sandiford commented on EXEC-63: -------------------------------------- Patch for test case from https://github.com/msandiford/commons-exec commit 0c7e9457f31ea0322d18dcb157d5192a25f0e490 {code:title=fix.diff|borderStyle=solid} --- a/src/main/java/org/apache/commons/exec/DefaultExecutor.java +++ b/src/main/java/org/apache/commons/exec/DefaultExecutor.java @@ -161,7 +161,7 @@ public class DefaultExecutor implements Executor { throw new IOException(workingDirectory + " doesn't exist."); } - return executeInternal(command, environment, workingDirectory, streamHandler); + return executeInternal(command, null, environment, workingDirectory, streamHandler); } @@ -189,13 +189,14 @@ public class DefaultExecutor implements Executor { watchdog.setProcessNotStarted(); } + final SimpleCondition condition = new SimpleCondition(); Runnable runnable = new Runnable() { public void run() { int exitValue = Executor.INVALID_EXITVALUE; try { - exitValue = executeInternal(command, environment, workingDirectory, streamHandler); + exitValue = executeInternal(command, condition, environment, workingDirectory, streamHandler); handler.onProcessComplete(exitValue); } catch (ExecuteException e) { handler.onProcessFailed(e); @@ -207,6 +208,9 @@ public class DefaultExecutor implements Executor { this.executorThread = createThread(runnable, "Exec Default Executor"); getExecutorThread().start(); + + // Wait until the thread tells us we have actually started + condition.sleep(); } /** @see org.apache.commons.exec.Executor#setExitValue(int) */ @@ -316,6 +320,33 @@ public class DefaultExecutor implements Executor { } /** + * Simple thread notify mechanism. + */ + private static final class SimpleCondition { + private final Object lock = new Object(); + private volatile boolean notified = false; + + public void sleep() { + try { + synchronized (lock) { + while (!notified) { + lock.wait(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + public void wakeup() { + synchronized (lock) { + notified = true; + lock.notifyAll(); + } + } + } + + /** * Execute an internal process. If the executing thread is interrupted while waiting for the * child process to return the child process will be killed. * @@ -326,12 +357,20 @@ public class DefaultExecutor implements Executor { * @return the exit code of the process * @throws IOException executing the process failed */ - private int executeInternal(final CommandLine command, final Map environment, - final File dir, final ExecuteStreamHandler streams) throws IOException { + private int executeInternal(final CommandLine command, final SimpleCondition cond, + final Map environment, final File dir, final ExecuteStreamHandler streams) + throws IOException { setExceptionCaught(null); - final Process process = this.launch(command, environment, dir); + final Process process; + try { + process = this.launch(command, environment, dir); + } finally { + // If necessary, let our parent know that the process has launched + if (cond != null) + cond.wakeup(); + } try { streams.setProcessInputStream(process.getOutputStream()); {code} > Race condition in DefaultExecutor#execute(cmd, handler) > ------------------------------------------------------- > > Key: EXEC-63 > URL: https://issues.apache.org/jira/browse/EXEC-63 > Project: Commons Exec > Issue Type: Bug > Affects Versions: 1.1 > Environment: Windows 7/64 bit, JDK 1.6.0_27 > Reporter: Martin Sandiford > Labels: deadlock > Original Estimate: 24h > Remaining Estimate: 24h > > {{DefaultExecutor#execute(CommandLine, ExecuteResultHandler)}} can, and > usually does, return before the target process has actually started. This > can result in a race condition where several asynchronous processes are > coupled by {{PipedInputStream}}/{{PipedOutputStream}} objects. > The following example shows the issue: > {code:title=Borken.java|borderStyle=solid} > import java.io.*; > import org.apache.commons.exec.*; > public class Borken { > public static int pipe(OutputStream out, OutputStream err, InputStream in, > CommandLine cmd0, CommandLine cmd1) > throws IOException { > PipedInputStream pipeIn = new PipedInputStream(); > PipedOutputStream pipeOut = new PipedOutputStream(pipeIn); > DefaultExecutor exec0 = new DefaultExecutor(); > exec0.setStreamHandler(new PumpStreamHandler(pipeOut, null, in)); > exec0.execute(cmd0, new DefaultExecuteResultHandler()); > > // If the following line is commented, deadlock occurs > //try { Thread.sleep(100); } catch (InterruptedException e) { } > DefaultExecutor exec1 = new DefaultExecutor(); > exec1.setStreamHandler(new PumpStreamHandler(out, err, pipeIn)); > return exec1.execute(cmd1); > } > > public static void main(String... args) { > CommandLine cmd0 = new > CommandLine("cmd").addArgument("/c").addArgument("dir"); > //CommandLine cmd0 = new CommandLine("ls").addArgument("-l"); > CommandLine cmd1 = new CommandLine("sort"); > ByteArrayOutputStream out = new ByteArrayOutputStream(); > ByteArrayOutputStream err = new ByteArrayOutputStream(); > ByteArrayInputStream in = new ByteArrayInputStream(new byte[0]); > try { > int result = pipe(out, err, in, cmd0, cmd1); > System.out.format("Result code: %d%n", result); > System.out.format("Out: %s%n", out.toString()); > } catch (Exception e) { > e.printStackTrace(); > } > } > } > {code} > One possible solution is to pass in a semaphore object into > {{DefaultExecutor#executeInternal}} which is notified once the process is > started. The {{execute(CommandLine, Map, ExecuteResultHandler)}} method can > then wait on this before returning. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira