You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Martin Sandiford (Issue Comment Edited) (JIRA)" <ji...@apache.org> on 2012/02/28 02:27:49 UTC
[jira] [Issue Comment Edited] (EXEC-63) Race condition in
DefaultExecutor#execute(cmd, handler)
[ https://issues.apache.org/jira/browse/EXEC-63?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13217797#comment-13217797 ]
Martin Sandiford edited comment on EXEC-63 at 2/28/12 1:26 AM:
---------------------------------------------------------------
Patch for fix 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}
was (Author: msandiford):
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