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 = *
