Commits

Michael Heemskerk committed 8280bf5

STASHDEV-2221 Ensure ProcessMonitor callbacks are always called

Previously, the PocessMonitor callbacks were not called when using ExternalProcess.start/
finish.

Also did a bit of refactoring of the unit tests to extract test utility code.

  • Participants
  • Parent commits 929dc3d
  • Branches STASHDEV-2221-processmonitor

Comments (0)

Files changed (5)

             <version>4.10</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>1.8.5</version>
+        </dependency>
     </dependencies>
 </project>

src/main/java/com/atlassian/utils/process/ExternalProcessImpl.java

     private LatchedRunnable errorPump;
     private final ExecutorService executorService;
     private long executionTimeout;
+    private AtomicBoolean finished = new AtomicBoolean(false);
     private ProcessHandler handler;
     private long idleTimeout;
     private LatchedRunnable inputPump;
      */
     public void start() {
         try {
+            notifyBeforeStart();
             this.startTime = System.currentTimeMillis();
             this.process = createProcess(command, environment, workingDir);
             setupIOPumps();
             LOG.debug(handlerName + ": Process canceled; ignoring exception", t);
         }
     }
-    
+
     private void setupIOPumps() {
         // set up threads to feed data to and extract data from the process
         if (handler.hasInput()) {
                     handler.processOutput(process.getInputStream());
                 } catch (Throwable t) {
                     handleHandlerError(name, t);
-                } 
+                }
             }
         };
 
     }
 
     /**
-     * Finish process execution. This method should be called after you have called the
-     * start() method.
-     */
-    public void finish() {
-        if (process != null) {
-            try {
-                do {
-                    long checkTime = getTimeoutTime();
-
-                    // block on the output pumps even when the process has already finished.
-                    // this gives the output pumps the chance to finish processing any output
-                    // still stuck in buffers
-                    awaitPump(outputPump, checkTime);
-                    awaitPump(errorPump, checkTime);
-
-                    // don't block on the input pump when the process has already finished
-                    awaitPumpOrProcess(inputPump, checkTime);
-                } while (!isTimedOut() && areOutputPumpsRunning() && !Thread.currentThread().isInterrupted());
-            } finally {
-                if (Thread.currentThread().isInterrupted()) {
-                    cancel();
-
-                    // All is good, now clearing interrupted state of current thread.
-                    Thread.interrupted();
-                }
-
-                int exitCode = wrapUpProcess();
-                handler.complete(exitCode, isCanceled(), processException);
-            }
-        } else {
-            handler.complete(-1, false, processException);
-        }
-    }
-
-    /**
      * Notifies all ProcessMonitors of the 'beforeStart' event.
      */
     private void notifyBeforeStart() {
         }
     }
 
+    public String getCommandLine() {
+        StringBuilder builder = new StringBuilder();
+        for (String s : command) {
+            if (builder.length() > 0) {
+                builder.append(" ");
+            }
+            builder.append(s);
+        }
+        return builder.toString();
+    }
+
     /**
      * Execute the external command. When this method returns, the process handler
      * provided at construction time should be consulted to collect exit code, exceptions,
      * process output, etc.
      */
     public void execute() {
-        notifyBeforeStart();
-        try {
-            start();
-            finish();
-        } finally {
-            notifyAfterFinished();
-        }
+        start();
+        finish();
     }
 
     /**
         finish();
     }
 
-    public String getCommandLine() {
-        StringBuilder builder = new StringBuilder();
-        for (String s : command) {
-            if (builder.length() > 0) {
-                builder.append(" ");
+    /**
+     * Finish process execution. This method should be called after you have called the
+     * start() method.
+     */
+    public void finish() {
+        if (finished.get()) {
+            return;
+        }
+
+        try {
+            do {
+                long checkTime = getTimeoutTime();
+
+                // block on the output pumps even when the process has already finished.
+                // this gives the output pumps the chance to finish processing any output
+                // still stuck in buffers
+                awaitPump(outputPump, checkTime);
+                awaitPump(errorPump, checkTime);
+
+                // don't block on the input pump when the process has already finished
+                awaitPumpOrProcess(inputPump, checkTime);
+            } while (!isTimedOut() && areOutputPumpsRunning() && !Thread.currentThread().isInterrupted());
+        } finally {
+            if (Thread.currentThread().isInterrupted()) {
+                cancel();
+
+                // All is good, now clearing interrupted state of current thread.
+                Thread.interrupted();
             }
-            builder.append(s);
+            wrapUpProcess();
         }
-        return builder.toString();
     }
 
     /**
      * @return true if the process has finished.
      */
     public boolean finish(int maxWait) {
-        if (process != null) {
-            boolean finished = false;
-            try {
-                long endTime = System.currentTimeMillis() + maxWait;
-                awaitPump(outputPump, endTime);
-                awaitPump(errorPump, endTime);
-                awaitPumpOrProcess(inputPump, endTime);
-            } finally {
-                if (!areOutputPumpsRunning() || !isAlive()) {
-                    // process finished
-                    finished = true;
-                    int exitCode = wrapUpProcess();
-                    handler.complete(exitCode, isCanceled(), processException);
-                }
-            }
-            return finished;
-        } else {
-            handler.complete(-1, false, processException);
+        if (finished.get()) {
             return true;
         }
+
+        try {
+            long endTime = System.currentTimeMillis() + maxWait;
+            awaitPump(outputPump, endTime);
+            awaitPump(errorPump, endTime);
+            awaitPumpOrProcess(inputPump, endTime);
+        } finally {
+            if (!areOutputPumpsRunning() || !isAlive()) {
+                // process finished
+                wrapUpProcess();
+            }
+        }
+        return finished.get();
     }
 
     private boolean shouldIgnoreInputPumpException(Throwable t) {
         return false;
     }
     
-    private int wrapUpProcess() {
+    private void wrapUpProcess() {
         int exitCode = -1;
-        boolean processIncomplete = true;
-        boolean interrupted = false;
-        try {
-            exitCode = process.exitValue();
-            processIncomplete = false;
-        } catch (IllegalThreadStateException itse) {
-            // process still running - could be a race to have the process finish so wait a little to be sure
-            while (processIncomplete && System.currentTimeMillis() - getTimeoutTime() < 10) {
-                // we are currently before the end of the period (within 10ms slack), so process probably not ready yet
-                try {
-                    Thread.sleep(100);
-                    exitCode = process.exitValue();
-                    processIncomplete = false;
-                } catch (InterruptedException ie) {
-                    processIncomplete = true;
-                    interrupted = true;
-                    break;
-                } catch (IllegalThreadStateException e) {
-                    // ignore and try in the next loop
+        if (process != null) {
+            boolean processIncomplete = true;
+            boolean interrupted = false;
+            try {
+                exitCode = process.exitValue();
+                processIncomplete = false;
+            } catch (IllegalThreadStateException itse) {
+                // process still running - could be a race to have the process finish so wait a little to be sure
+                while (processIncomplete && System.currentTimeMillis() - getTimeoutTime() < 10) {
+                    // we are currently before the end of the period (within 10ms slack), so process probably not ready yet
+                    try {
+                        Thread.sleep(100);
+                        exitCode = process.exitValue();
+                        processIncomplete = false;
+                    } catch (InterruptedException ie) {
+                        processIncomplete = true;
+                        interrupted = true;
+                        break;
+                    } catch (IllegalThreadStateException e) {
+                        // ignore and try in the next loop
+                    }
                 }
+            } finally {
+                internalCancel(exitCode);
             }
-        } finally {
-            internalCancel(exitCode);
+
+            if (processIncomplete && !interrupted) {
+                processException = new ProcessTimeoutException("process timed out");
+            }
         }
 
-        if (processIncomplete && !interrupted) {
-            processException = new ProcessTimeoutException("process timed out");
-        }
-        return exitCode;
+        handler.complete(exitCode, isCanceled(), processException);
+        finished.set(true);
+
+        notifyAfterFinished();
     }
 
     /**
                     LOG.error("Failed to kill Windows process; falling back on Process.destroy()", t);
                     process.destroy();
                 }
-
             } else {
                 process.destroy();
             }

src/test/java/com/atlassian/utils/process/ExternalProcessImplTest.java

 
 import org.junit.Test;
 
-import java.io.OutputStream;
 import java.util.Arrays;
 import java.util.concurrent.TimeUnit;
 
 import static org.junit.Assert.*;
+import static com.atlassian.utils.process.ExternalProcessTestUtils.*;
 
+@SuppressWarnings("ThrowableResultOfMethodCallIgnored")
 public class ExternalProcessImplTest {
 
-    @Test(timeout = 10 * 1000) //Test timeout in case execution doesn't timeout like it should
+    @Test(timeout = 3 * 1000) //Test timeout in case execution doesn't timeout like it should
     public void testExecutionTimeout() {
-        //Using ping because the command itself is cross-platform (existence-wise) and it produces a steady, continuous
-        //flow of output. That allows the StringOutputHandler to prevent the process from ever idling into a timeout so
-        //we can test the overall execution timeout
-        ExternalProcessBuilder builder = new ExternalProcessBuilder();
-        if (isWindows()) {
-            //ping on Windows requires -t to make it continuous
-            builder.command(Arrays.asList("ping", "-t", "localhost"));
-        } else {
-            //ping on non-Windows OS's is continuous by default
-            builder.command(Arrays.asList("ping", "localhost"));
-        }
-        //Ensure the execution timeout here is less than the timeout on the test
-        ExternalProcess process = builder.executionTimeout(TimeUnit.SECONDS.toMillis(5))
+        ExternalProcess process = createProcessBuilderForExecutionTimeoutTests(1)
                 .handlers(new StringOutputHandler()) //Will ensure we don't idle out
                 .build();
         process.execute();
         assertTrue(exception instanceof ProcessTimeoutException);
     }
 
-    @Test(timeout = 10 * 1000) //Test timeout in case execution doesn't stop timeout like it should
+    @Test(timeout = 3 * 1000) //Test timeout in case execution doesn't stop timeout like it should
     public void testIdleTimeout() {
-        //Use a command which produces no output and will run for a long time
-        ExternalProcessBuilder builder = new ExternalProcessBuilder();
-        if (isWindows()) {
-            //pause produces a small amount of output immediately but then produces no more. It will run until it
-            //receives a keystroke (which it never will since we provide no input handler)
-            builder.command(Arrays.asList("pause"));
-        } else {
-            //sleep is present on MacOS and Linux and produces no output while running for a specified duration. We
-            //provide a massive duration here so that we know the command will never complete before idling out
-            builder.command(Arrays.asList("sleep", String.valueOf(TimeUnit.MINUTES.toSeconds(30))));
-        }
         //Ensure the timeout here is less than the timeout on the test
-        ExternalProcess process = builder.idleTimeout(TimeUnit.SECONDS.toMillis(5))
+        ExternalProcess process = createProcessBuilderForIdleTimeoutTests(500L, TimeUnit.MILLISECONDS)
                 .handlers(new StringOutputHandler())
                 .build();
         process.execute();
     }
 
     private ExternalProcess buildProcessWithBlockingInputHandler() {
-        ExternalProcessBuilder builder = new ExternalProcessBuilder();
-        if (isWindows()) {
-            //pause produces a small amount of output immediately but then produces no more. 
-            builder.command(Arrays.asList("pause"));
-
-        } else {
-            //sleep is present on MacOS and Linux and produces no output while running for a specified duration. We
-            //provide a short duration here so that we know the command will 
-            builder.command(Arrays.asList("sleep", "1"));
-        }
-
-
-        InputHandler inputHandler = new BaseInputHandler() {
-            public void process(OutputStream input) {
-                try {
-                    // this is for windows pause. Wait 1 second then press a key
-                    input.write("key".getBytes());
-                    input.flush();
-
-                    // sleep for a long time, the process should finish before the inputHandler finishes
-                    // this simulates an inputHandler that simply copies data from some other stream into
-                    // the process inputstream. It is quite common for the process to finish while the
-                    // inputHandler is blocked on IO.
-                    Thread.sleep(30 * 1000L);
-                } catch (Exception e) {
-                    throw new RuntimeException(e);
-                }
-            }
-        };
-
-        //Ensure the timeout here is less than the timeout on the test
-        return builder.idleTimeout(TimeUnit.SECONDS.toMillis(15))
-                .handlers(inputHandler, new StringOutputHandler())
+        return createProcessBuilderThatExecutesFor(1)
+                .handlers(createBlockingInputHandler(10, TimeUnit.SECONDS), new StringOutputHandler())
                 .build();
     }
 
-    @Test(timeout = 5 * 1000) // Test timeout in case execution doesn't stop as it should
+    @Test(timeout = 2 * 1000) // Test timeout in case execution doesn't stop as it should
     public void testBlockingInputHandlerAbortsWhenProcessFinishes() {
         ExternalProcess process = buildProcessWithBlockingInputHandler();
 
         assertEquals(0, process.getHandler().getExitCode());
     }
 
-    @Test(timeout = 5 * 1000) // Test timeout in case execution doesn't stop as it should
+    @Test(timeout = 2 * 1000) // Test timeout in case execution doesn't stop as it should
     public void testBlockingInputHandlerAbortsWhenProcessFinishes2() {
         ExternalProcess process = buildProcessWithBlockingInputHandler();
 
         assertEquals(0, process.getHandler().getExitCode());
     }
 
-    @Test
+    @Test(timeout = 3 * 1000)
     public void testNativeWindowsKilling() {
         if (isWindows()) {
             ExternalProcess process = new ExternalProcessBuilder()
         }
     }
 
+    /**
+     * Not sure whether this code path makes a whole lot of sense, but there's quite a bit of code for dealing with
+     * this condition, so better to test that it works..
+     */
+    @Test(timeout = 250)
+    public void testFinishWithoutStart() {
+        ExternalProcess process = buildProcessWithBlockingInputHandler();
+
+        process.finish();
+    }
+
+    @Test(timeout = 250)
+    public void testFinishWithTimeoutWithoutStart() {
+        ExternalProcess process = buildProcessWithBlockingInputHandler();
+
+        assertTrue(process.finish(100));
+    }
+
     private boolean isWindows() {
         return System.getProperty("os.name").toLowerCase().contains("windows");
     }

src/test/java/com/atlassian/utils/process/ExternalProcessMonitorTest.java

+package com.atlassian.utils.process;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import java.io.InputStream;
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+
+import static com.atlassian.utils.process.ExternalProcessTestUtils.*;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.same;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+/**
+ * Unit test for testing the ProcessMonitor contract: onBeforeStart and onAfterFinished should be called for both
+ * the {@link com.atlassian.utils.process.ExternalProcess#execute()} and
+ * {@link com.atlassian.utils.process.ExternalProcess#start()} / {@link com.atlassian.utils.process.ExternalProcess#finish()}
+ * modes of usage.
+ * <p/>
+ * Also, the {@link ProcessMonitor#onAfterFinished(ExternalProcess)} must be called, even if there's a timeout or
+ * exception.
+ */
+@SuppressWarnings("ThrowableResultOfMethodCallIgnored")
+@RunWith(MockitoJUnitRunner.class)
+public class ExternalProcessMonitorTest {
+
+    @Mock
+    private ProcessMonitor monitor;
+
+    private ExternalProcess process;
+
+    @Before
+    public void createProcess() {
+        process = new ExternalProcessBuilder()
+                .command(Arrays.asList("echo", "haha"))
+                .addMonitor(monitor)
+                .handlers(new StringOutputHandler())
+                .build();
+    }
+
+    @Test
+    public void testMonitorCalledOnExecute() {
+        process.execute();
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnExecuteWhile() {
+        process.executeWhile(new Runnable() {
+            public void run() {
+                try {
+                    Thread.sleep(500L);
+                } catch (InterruptedException e) {
+                    // ignore
+                }
+            }
+        });
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+
+    @Test
+    public void testMonitorCalledOnStartFinish() {
+        process.start();
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor, never()).onAfterFinished(same(process));
+
+        process.finish();
+
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnStartFinishWait() {
+        process = createProcessBuilderForExecutionTimeoutTests(1)
+                .addMonitor(monitor)
+                .handlers(new StringOutputHandler())
+                .build();
+        process.start();
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor, never()).onAfterFinished(same(process));
+
+        assertFalse(process.finish(100));
+
+        verify(monitor, never()).onAfterFinished(same(process));
+
+        assertTrue(process.finish(1000));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnExecutionTimeout() {
+        process = createProcessBuilderForExecutionTimeoutTests(1)
+                .addMonitor(monitor)
+                .handlers(new StringOutputHandler())
+                .build();
+
+        process.execute();
+
+        assertTrue(process.getHandler().getException() instanceof ProcessTimeoutException);
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnIdleTimeout() {
+        process = createProcessBuilderForIdleTimeoutTests(500, TimeUnit.MILLISECONDS)
+                .addMonitor(monitor)
+                .handlers(new StringOutputHandler())
+                .build();
+
+        process.execute();
+
+        assertTrue(process.getHandler().getException() instanceof ProcessTimeoutException);
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnProcessException() {
+        process = createProcessThatThrowsException(new ProcessException("Oh no"));
+
+        process.execute();
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    @Test
+    public void testMonitorCalledOnRuntimeException() {
+        process = createProcessThatThrowsException(new RuntimeException("Oh no"));
+
+        process.execute();
+
+        verify(monitor).onBeforeStart(same(process));
+        verify(monitor).onAfterFinished(same(process));
+    }
+
+    private ExternalProcess createProcessThatThrowsException(final Exception e) {
+        return createProcessBuilderThatExecutesFor(1)
+                .addMonitor(monitor)
+                .handlers(new BaseOutputHandler() {
+                    public void process(InputStream output) throws ProcessException {
+                        if (e instanceof RuntimeException) {
+                            throw (RuntimeException) e;
+                        }
+
+                        throw new ProcessException(e);
+                    }
+                })
+                .build();
+    }
+
+}

src/test/java/com/atlassian/utils/process/ExternalProcessTestUtils.java

+package com.atlassian.utils.process;
+
+import java.io.OutputStream;
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Utility methods for testing ExternalProcess.
+ */
+public class ExternalProcessTestUtils {
+    /**
+     * @param blockTime the time to block on input processing
+     * @param timeUnit the time unit
+     * @return an InputHandler that writes a single character and then blocks for a while
+     */
+    public static InputHandler createBlockingInputHandler(final long blockTime, final TimeUnit timeUnit) {
+        return new BaseInputHandler() {
+            public void process(OutputStream input) {
+                try {
+                    // this is for windows pause. Wait 1 second then press a key
+                    input.write("k".getBytes());
+                    input.flush();
+
+                    // sleep for a long time, the process should finish before the inputHandler finishes
+                    // this simulates an inputHandler that simply copies data from some other stream into
+                    // the process inputstream. It is quite common for the process to finish while the
+                    // inputHandler is blocked on IO.
+                    Thread.sleep(timeUnit.toMillis(blockTime));
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        };
+    }
+
+    /**
+     * Using ping because the command itself is cross-platform (existence-wise) and it produces a steady, continuous
+     * flow of output. That allows a StringOutputHandler to prevent the process from ever idling into a timeout so
+     * we can test the overall execution timeout
+     *
+     * @param count the number of ping requests that need to be attempted.
+     * @param timeoutMillis the time to wait between sending ping requests.
+     * @param target the hostname / ip address to ping
+     * @return an ExternalProcessBuilder for the ping command
+     */
+    public static ExternalProcessBuilder createPingBuilder(int count, long timeoutMillis, String target) {
+        ExternalProcessBuilder builder = new ExternalProcessBuilder();
+        String countString = Integer.toString(count);
+        String timeoutString = Long.toString(timeoutMillis);
+        if (isWindows()) {
+            builder.command(Arrays.asList("ping", "-n", countString, "-w", timeoutString, target));
+        } else {
+            builder.command(Arrays.asList("ping", "-c", countString, "-W", timeoutString, target));
+        }
+        return builder;
+    }
+
+    public static ExternalProcessBuilder createProcessBuilderForExecutionTimeoutTests(int durationSeconds) {
+        // ensure the process executes a bit longer than the execution timeout
+        return createProcessBuilderThatExecutesFor(durationSeconds)
+                .executionTimeout(TimeUnit.SECONDS.toMillis(durationSeconds) - 250)
+                .idleTimeout(TimeUnit.SECONDS.toMillis(durationSeconds) + 250);
+
+    }
+
+    /**
+     * @return a ExternalProcessBuilder for a command that will run for {@code duration}
+     */
+    public static ExternalProcessBuilder createProcessBuilderThatExecutesFor(int durationSeconds) {
+        // 1 ping per second; we need n+1 pings to execute for n seconds
+        int pingCount = durationSeconds + 1;
+        return createPingBuilder(pingCount, 1000 * durationSeconds, "localhost");
+    }
+
+    /**
+     * @return an ExternalProcessBuilder with that will wait for {@code duration} without providing any output
+     */
+    public static ExternalProcessBuilder createProcessBuilderThatIdlesFor(long duration, TimeUnit timeUnit) {
+        // ping to a non-existent ip address to force it to idle processing
+        // note that the exit code will be 2, so the command will be marked as failed.
+        return createPingBuilder(1, timeUnit.toMillis(duration), "123.45.67.89");
+    }
+
+    public static ExternalProcessBuilder createProcessBuilderForIdleTimeoutTests(long duration, TimeUnit timeUnit) {
+        // ensure the process idles a bit longer than the idle timeout
+        long durationMillis = timeUnit.toMillis(duration);
+        return createProcessBuilderThatIdlesFor(durationMillis + 100, TimeUnit.MILLISECONDS)
+                .idleTimeout(durationMillis)
+                .executionTimeout(durationMillis + 250L);
+    }
+
+    private static boolean isWindows() {
+        return System.getProperty("os.name").toLowerCase().contains("windows");
+    }
+
+}