commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Sandiford (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (EXEC-63) Race condition in DefaultExecutor#execute(cmd, handler)
Date Tue, 28 Feb 2012 01:27:48 GMT

    [ 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

        

Mime
View raw message