commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sgoes...@apache.org
Subject svn commit: r1181250 - in /commons/proper/exec/trunk/src: main/java/org/apache/commons/exec/ test/java/org/apache/commons/exec/ test/scripts/
Date Mon, 10 Oct 2011 21:33:07 GMT
Author: sgoeschl
Date: Mon Oct 10 21:33:07 2011
New Revision: 1181250

URL: http://svn.apache.org/viewvc?rev=1181250&view=rev
Log:
[EXEX-57] - Applied the patch from Nickolay Martinov but the timeout disguises the fact that
the process might be still runnung - therefore added a sanity check in order to throw an exception
if the the timeout for join() was exceeded.

Added:
    commons/proper/exec/trunk/src/test/scripts/invoker.sh   (with props)
Modified:
    commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/DefaultExecutor.java
    commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/ExecuteStreamHandler.java
    commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/PumpStreamHandler.java
    commons/proper/exec/trunk/src/test/java/org/apache/commons/exec/DefaultExecutorTest.java

Modified: commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/DefaultExecutor.java
URL: http://svn.apache.org/viewvc/commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/DefaultExecutor.java?rev=1181250&r1=1181249&r2=1181250&view=diff
==============================================================================
--- commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/DefaultExecutor.java (original)
+++ commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/DefaultExecutor.java Mon
Oct 10 21:33:07 2011
@@ -66,6 +66,9 @@ public class DefaultExecutor implements 
     /** worker thread for asynchronous execution */
     private Thread executorThread;
 
+    /** the first exception being caught to be thrown to the caller */
+    private IOException exceptionCaught;
+
     /**
      * Default constructor creating a default <code>PumpStreamHandler</code>
      * and sets the working directory of the subprocess to the current
@@ -81,6 +84,7 @@ public class DefaultExecutor implements 
         this.launcher = CommandLauncherFactory.createVMLauncher();
         this.exitValues = new int[0];
         this.workingDirectory = new File(".");
+        this.exceptionCaught = null;
     }
 
     /**
@@ -279,50 +283,40 @@ public class DefaultExecutor implements 
     }
     
     /**
-     * Close the streams belonging to the given Process. In the
-     * original implementation all exceptions were dropped which
-     * is probably not a good thing. On the other hand the signature
-     * allows throwing an IOException so the current implementation
-     * might be quite okay.
-     * 
+     * Close the streams belonging to the given Process.
+     *
      * @param process the <CODE>Process</CODE>.
-     * @throws IOException closing one of the three streams failed
      */
-    private void closeStreams(final Process process) throws IOException {
-
-        IOException caught = null;
+    private void closeProcessStreams(final Process process) {
 
         try {
             process.getInputStream().close();
         }
         catch(IOException e) {
-            caught = e;
+            setExceptionCaught(e);
         }
 
         try {
             process.getOutputStream().close();
         }
         catch(IOException e) {
-            caught = e;
+            setExceptionCaught(e);
         }
 
         try {
             process.getErrorStream().close();
         }
         catch(IOException e) {
-            caught = e;
-        }
-
-        if(caught != null) {
-            throw caught;
+            setExceptionCaught(e);
         }
     }
 
     /**
-     * Execute an internal process.
+     * Execute an internal process. If the executing thread is interrupted while waiting
for the
+     * child process to return the child process will be killed.
      *
      * @param command the command to execute
-     * @param environment the execution enviroment
+     * @param environment the execution environment
      * @param dir the working directory
      * @param streams process the streams (in, out, err) of the process
      * @return the exit code of the process
@@ -331,6 +325,8 @@ public class DefaultExecutor implements 
     private int executeInternal(final CommandLine command, final Map environment,
             final File dir, final ExecuteStreamHandler streams) throws IOException {
 
+        setExceptionCaught(null);
+
         final Process process = this.launch(command, environment, dir);
 
         try {
@@ -375,8 +371,18 @@ public class DefaultExecutor implements 
                 watchdog.stop();
             }
 
-            streams.stop();
-            closeStreams(process);
+            try {
+                streams.stop();
+            }
+            catch(IOException e) {
+                setExceptionCaught(e);
+            }
+
+            closeProcessStreams(process);
+
+            if(getExceptionCaught() != null) {
+                throw getExceptionCaught();
+            }
 
             if (watchdog != null) {
                 try {
@@ -400,4 +406,25 @@ public class DefaultExecutor implements 
             }
         }
     }
+
+    /**
+     * Keep track of the first IOException being thrown.
+     *
+     * @param e the IOException
+     */
+    private void setExceptionCaught(IOException e) {
+        if(this.exceptionCaught == null) {
+            this.exceptionCaught = e;
+        }
+    }
+
+    /**
+     * Get the first IOException being thrown.
+     *
+     * @return the first IOException being caught
+     */
+    private IOException getExceptionCaught() {
+        return this.exceptionCaught;
+    }
+
 }

Modified: commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/ExecuteStreamHandler.java
URL: http://svn.apache.org/viewvc/commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/ExecuteStreamHandler.java?rev=1181250&r1=1181249&r2=1181250&view=diff
==============================================================================
--- commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/ExecuteStreamHandler.java
(original)
+++ commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/ExecuteStreamHandler.java
Mon Oct 10 21:33:07 2011
@@ -62,5 +62,5 @@ public interface ExecuteStreamHandler {
      * Stop handling of the streams - will not be restarted.
      * Will wait for pump threads to complete.
      */
-    void stop();
+    void stop() throws IOException;
 }

Modified: commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/PumpStreamHandler.java
URL: http://svn.apache.org/viewvc/commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/PumpStreamHandler.java?rev=1181250&r1=1181249&r2=1181250&view=diff
==============================================================================
--- commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/PumpStreamHandler.java
(original)
+++ commons/proper/exec/trunk/src/main/java/org/apache/commons/exec/PumpStreamHandler.java
Mon Oct 10 21:33:07 2011
@@ -26,12 +26,14 @@ import java.io.OutputStream;
 import java.io.PipedOutputStream;
 
 /**
- * Copies standard output and error of subprocesses to standard output and error
+ * Copies standard output and error of sub-processes to standard output and error
  * of the parent process. If output or error stream are set to null, any feedback
- * from that stream will be lost. 
+ * from that stream will be lost.
  */
 public class PumpStreamHandler implements ExecuteStreamHandler {
 
+    private static final long STOP_TIMEOUT_ADDITION = 2000L;
+
     private Thread outputThread;
 
     private Thread errorThread;
@@ -45,7 +47,13 @@ public class PumpStreamHandler implement
     private final InputStream input;
 
     private InputStreamPumper inputStreamPumper;
-    
+
+    /** the timeout in ms the implementation waits when stopping the pumper threads */
+    private long stopTimeout;
+
+    /** the last exception being caught */
+    private IOException caught = null;
+
     /**
      * Construct a new <CODE>PumpStreamHandler</CODE>.
      */
@@ -56,20 +64,17 @@ public class PumpStreamHandler implement
     /**
      * Construct a new <CODE>PumpStreamHandler</CODE>.
      *
-     * @param outAndErr
-     *            the output/error <CODE>OutputStream</CODE>.
+     * @param outAndErr the output/error <CODE>OutputStream</CODE>.
      */
     public PumpStreamHandler(final OutputStream outAndErr) {
         this(outAndErr, outAndErr);
     }
-    
+
     /**
      * Construct a new <CODE>PumpStreamHandler</CODE>.
      *
-     * @param out
-     *            the output <CODE>OutputStream</CODE>.
-     * @param err
-     *            the error <CODE>OutputStream</CODE>.
+     * @param out the output <CODE>OutputStream</CODE>.
+     * @param err the error <CODE>OutputStream</CODE>.
      */
     public PumpStreamHandler(final OutputStream out, final OutputStream err) {
         this(out, err, null);
@@ -77,28 +82,32 @@ public class PumpStreamHandler implement
 
     /**
      * Construct a new <CODE>PumpStreamHandler</CODE>.
-     * 
-     * @param out
-     *            the output <CODE>OutputStream</CODE>.
-     * @param err
-     *            the error <CODE>OutputStream</CODE>.
-     * @param input
-     *            the input <CODE>InputStream</CODE>.
+     *
+     * @param out   the output <CODE>OutputStream</CODE>.
+     * @param err   the error <CODE>OutputStream</CODE>.
+     * @param input the input <CODE>InputStream</CODE>.
      */
-    public PumpStreamHandler(final OutputStream out, final OutputStream err,
-            final InputStream input) {
-
+    public PumpStreamHandler(final OutputStream out, final OutputStream err, final InputStream
input) {
         this.out = out;
         this.err = err;
         this.input = input;
     }
 
     /**
+     * Set maximum time to wait until output streams are exchausted
+     * when {@link #stop()} was called.
+     *
+     * @param timeout timeout in milliseconds or zero to wait forever (default)
+     */
+    public void setStopTimeout(long timeout) {
+        this.stopTimeout = timeout;
+    }
+
+    /**
      * Set the <CODE>InputStream</CODE> from which to read the standard output
      * of the process.
-     * 
-     * @param is
-     *            the <CODE>InputStream</CODE>.
+     *
+     * @param is the <CODE>InputStream</CODE>.
      */
     public void setProcessOutputStream(final InputStream is) {
         if (out != null) {
@@ -109,9 +118,8 @@ public class PumpStreamHandler implement
     /**
      * Set the <CODE>InputStream</CODE> from which to read the standard error
      * of the process.
-     * 
-     * @param is
-     *            the <CODE>InputStream</CODE>.
+     *
+     * @param is the <CODE>InputStream</CODE>.
      */
     public void setProcessErrorStream(final InputStream is) {
         if (err != null) {
@@ -122,27 +130,26 @@ public class PumpStreamHandler implement
     /**
      * Set the <CODE>OutputStream</CODE> by means of which input can be sent
      * to the process.
-     * 
-     * @param os
-     *            the <CODE>OutputStream</CODE>.
+     *
+     * @param os the <CODE>OutputStream</CODE>.
      */
     public void setProcessInputStream(final OutputStream os) {
         if (input != null) {
             if (input == System.in) {
                 inputThread = createSystemInPump(input, os);
-        } else {
+            } else {
                 inputThread = createPump(input, os, true);
-            }        } 
-        else {
+            }
+        } else {
             try {
                 os.close();
             } catch (IOException e) {
                 String msg = "Got exception while closing output stream";
-                DebugUtils.handleException(msg ,e);
+                DebugUtils.handleException(msg, e);
             }
         }
     }
-        
+
     /**
      * Start the <CODE>Thread</CODE>s.
      */
@@ -159,63 +166,45 @@ public class PumpStreamHandler implement
     }
 
     /**
-     * Stop pumping the streams.
+     * Stop pumping the streams. When a timeout is specified it it is not guaranteed that
the
+     * pumper threads are cleanly terminated.
      */
-    public void stop() {
+    public void stop() throws IOException {
 
         if (inputStreamPumper != null) {
             inputStreamPumper.stopProcessing();
         }
 
-        if (outputThread != null) {
-            try {
-                outputThread.join();
-                outputThread = null;
-            } catch (InterruptedException e) {
-                // ignore
-            }
-        }
+        stopThread(outputThread, stopTimeout);
+        stopThread(errorThread, stopTimeout);
+        stopThread(inputThread, stopTimeout);
 
-        if (errorThread != null) {
+        if (err != null && err != out) {
             try {
-                errorThread.join();
-                errorThread = null;
-            } catch (InterruptedException e) {
-                // ignore
+                err.flush();
+            } catch (IOException e) {
+                String msg = "Got exception while flushing the error stream : " + e.getMessage();
+                DebugUtils.handleException(msg, e);
             }
         }
 
-        if (inputThread != null) {
+        if (out != null) {
             try {
-                inputThread.join();
-                inputThread = null;
-            } catch (InterruptedException e) {
-                // ignore
+                out.flush();
+            } catch (IOException e) {
+                String msg = "Got exception while flushing the output stream";
+                DebugUtils.handleException(msg, e);
             }
         }
 
-         if (err != null && err != out) {
-             try {
-                 err.flush();
-             } catch (IOException e) {
-                 String msg = "Got exception while flushing the error stream : " + e.getMessage();
-                 DebugUtils.handleException(msg ,e);
-             }
-         }
-
-         if (out != null) {
-             try {
-                 out.flush();
-             } catch (IOException e) {
-                 String msg = "Got exception while flushing the output stream";
-                 DebugUtils.handleException(msg ,e);
-             }
-         }
+        if(caught != null) {
+            throw caught;
+        }
     }
 
     /**
      * Get the error stream.
-     * 
+     *
      * @return <CODE>OutputStream</CODE>.
      */
     protected OutputStream getErr() {
@@ -224,7 +213,7 @@ public class PumpStreamHandler implement
 
     /**
      * Get the output stream.
-     * 
+     *
      * @return <CODE>OutputStream</CODE>.
      */
     protected OutputStream getOut() {
@@ -233,41 +222,34 @@ public class PumpStreamHandler implement
 
     /**
      * Create the pump to handle process output.
-     * 
-     * @param is
-     *            the <CODE>InputStream</CODE>.
-     * @param os
-     *            the <CODE>OutputStream</CODE>.
+     *
+     * @param is the <CODE>InputStream</CODE>.
+     * @param os the <CODE>OutputStream</CODE>.
      */
-    protected void createProcessOutputPump(final InputStream is,
-            final OutputStream os) {
+    protected void createProcessOutputPump(final InputStream is, final OutputStream os) {
         outputThread = createPump(is, os);
     }
 
     /**
      * Create the pump to handle error output.
-     * 
-     * @param is
-     *            the <CODE>InputStream</CODE>.
-     * @param os
-     *            the <CODE>OutputStream</CODE>.
+     *
+     * @param is the <CODE>InputStream</CODE>.
+     * @param os the <CODE>OutputStream</CODE>.
      */
-    protected void createProcessErrorPump(final InputStream is,
-            final OutputStream os) {
+    protected void createProcessErrorPump(final InputStream is, final OutputStream os) {
         errorThread = createPump(is, os);
     }
 
     /**
      * Creates a stream pumper to copy the given input stream to the given
      * output stream. When the 'os' is an PipedOutputStream we are closing
-     * 'os' afterwards to avoid an IOException ("Write end dead"). 
+     * 'os' afterwards to avoid an IOException ("Write end dead").
      *
      * @param is the input stream to copy from
      * @param os the output stream to copy into
      * @return the stream pumper thread
      */
     protected Thread createPump(final InputStream is, final OutputStream os) {
-
         boolean closeWhenExhausted = (os instanceof PipedOutputStream ? true : false);
         return createPump(is, os, closeWhenExhausted);
     }
@@ -276,20 +258,48 @@ public class PumpStreamHandler implement
      * Creates a stream pumper to copy the given input stream to the given
      * output stream.
      *
-     * @param is the input stream to copy from
-     * @param os the output stream to copy into
+     * @param is                 the input stream to copy from
+     * @param os                 the output stream to copy into
      * @param closeWhenExhausted close the output stream when the input stream is exhausted
      * @return the stream pumper thread
      */
-    protected Thread createPump(final InputStream is, final OutputStream os,
-            final boolean closeWhenExhausted) {
-        final Thread result = new Thread(new StreamPumper(is, os,
-                closeWhenExhausted), "Exec Stream Pumper");
+    protected Thread createPump(final InputStream is, final OutputStream os, final boolean
closeWhenExhausted) {
+        final Thread result = new Thread(new StreamPumper(is, os, closeWhenExhausted), "Exec
Stream Pumper");
         result.setDaemon(true);
         return result;
     }
 
     /**
+     * Stopping a pumper thread. The implementation actually waits
+     * longer than specified in 'timeout' to detect if the timeout
+     * was indeed exceeded. If the timeout was exceeded an IOException
+     * is created to be thrown to the caller.
+     *
+     * @param thread  the thread to be stopped
+     * @param timeout the time in ms to wait to join
+     */
+    protected void stopThread(Thread thread, long timeout) {
+
+        if (thread != null) {
+            try {
+                if (timeout == 0) {
+                    thread.join();
+                } else {
+                    long timeToWait = timeout + STOP_TIMEOUT_ADDITION;
+                    long startTime = System.currentTimeMillis();
+                    thread.join(timeToWait);
+                    if (!(System.currentTimeMillis() < startTime + timeToWait)) {
+                        String msg = "The stop timeout of " + timeout + " ms was exceeded";
+                        caught = new ExecuteException(msg, Executor.INVALID_EXITVALUE);
+                    }
+                }
+            } catch (InterruptedException e) {
+                thread.interrupt();
+            }
+        }
+    }
+
+    /**
      * Creates a stream pumper to copy the given input stream to the given
      * output stream.
      *
@@ -303,5 +313,4 @@ public class PumpStreamHandler implement
         result.setDaemon(true);
         return result;
     }
-
 }

Modified: commons/proper/exec/trunk/src/test/java/org/apache/commons/exec/DefaultExecutorTest.java
URL: http://svn.apache.org/viewvc/commons/proper/exec/trunk/src/test/java/org/apache/commons/exec/DefaultExecutorTest.java?rev=1181250&r1=1181249&r2=1181250&view=diff
==============================================================================
--- commons/proper/exec/trunk/src/test/java/org/apache/commons/exec/DefaultExecutorTest.java
(original)
+++ commons/proper/exec/trunk/src/test/java/org/apache/commons/exec/DefaultExecutorTest.java
Mon Oct 10 21:33:07 2011
@@ -54,6 +54,8 @@ public class DefaultExecutorTest extends
     private File acroRd32Script = TestUtil.resolveScriptForOS(testDir + "/acrord32");
     private File stdinSript = TestUtil.resolveScriptForOS(testDir + "/stdin");
     private File environmentSript = TestUtil.resolveScriptForOS(testDir + "/environment");
+    private File wrapperScript = TestUtil.resolveScriptForOS(testDir + "/wrapper");
+
 
     // Get suitable exit codes for the OS
     private static final int SUCCESS_STATUS; // test script successful exit code
@@ -1015,6 +1017,49 @@ public class DefaultExecutorTest extends
     }
 
     /**
+     * Test EXEC-57 (https://issues.apache.org/jira/browse/EXEC-57).
+     *
+     * DefaultExecutor.execute() does not return even if child process terminated - in this
+     * case the child process hangs because the grand children is connected to stdout &
stderr
+     * and is still running. As work-around a stop timeout is used for the PumpStreamHandler
+     * to ensure that the caller does not block forever but if the stop timeout is exceeded
+     * an ExecuteException is thrown to notify the caller.
+     *
+     * @throws Exception the test failed
+     */
+    public void testExec_57() throws IOException {
+
+        if(!OS.isFamilyUnix()) {
+            System.err.println("The test 'testSyncInvocationOfBackgroundProcess' does not
support the following OS : " + System.getProperty("os.name"));
+            return;
+        }
+
+        CommandLine cmdLine = new CommandLine("sh").addArgument("-c").addArgument(testDir
+ "/invoker.sh", false);
+
+        DefaultExecutor executor = new DefaultExecutor();
+        PumpStreamHandler pumpStreamHandler = new PumpStreamHandler(System.out, System.err);
+
+        // Without this timeout current thread will be blocked
+        // even if command we'll invoke will terminate immediately.
+        pumpStreamHandler.setStopTimeout(2000);
+        executor.setStreamHandler(pumpStreamHandler);
+        long startTime = System.currentTimeMillis();
+
+        System.out.println("Executing " + cmdLine);
+
+        try {
+            executor.execute(cmdLine);
+        }
+        catch(ExecuteException e) {
+            long duration = System.currentTimeMillis() - startTime;
+            System.out.println("Process completed in " + duration +" millis; above is its
output");
+            return;
+        }
+
+        fail("Expecting an ExecuteException");
+    }
+
+    /**
      * Test EXEC-60 (https://issues.apache.org/jira/browse/EXEC-60).
      *
      * Possible deadlock when a process is terminating at the same time its timing out. Please

Added: commons/proper/exec/trunk/src/test/scripts/invoker.sh
URL: http://svn.apache.org/viewvc/commons/proper/exec/trunk/src/test/scripts/invoker.sh?rev=1181250&view=auto
==============================================================================
--- commons/proper/exec/trunk/src/test/scripts/invoker.sh (added)
+++ commons/proper/exec/trunk/src/test/scripts/invoker.sh Mon Oct 10 21:33:07 2011
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+echo "invoker.sh -- going to start daemon process"
+cd ../../../target
+nohup sleep 10 &
+echo "invoker.sh --  daemon process was started"
\ No newline at end of file

Propchange: commons/proper/exec/trunk/src/test/scripts/invoker.sh
------------------------------------------------------------------------------
    svn:executable = *



Mime
View raw message