Commits

Bryan Turner committed e2d4cea

Deprecated ExternalProcessBuilder.timeout and ExternalProcessImpl.setTimeout
Created new ExternalProcessBuilder.executionTimeout and ExternalProcessBuilder.idleTimeout properties
- idleTimeout replaces the now-deprecated timeout property
- executionTimeout adds a hard cap on process execution duration even if the process is not idle
Created new ExternalProcessImpl.setExecutionTimeout and ExternalProcessImpl.setIdleTimeout properties
Updated ExternalProcessImpl.getTimeoutTime() to factor in the executionTimeout if set
Added unit tests (hopefully cross-platform) for testing both execution and idle timeouts

Comments (0)

Files changed (8)

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

         if (settings.hasEnvironment()) {
             process.setEnvironment(settings.getEnvironment());
         }
-        if (settings.hasTimeout()) {
-            process.setTimeout(settings.getTimeout());
+        if (settings.hasExecutionTimeout()) {
+            process.setExecutionTimeout(settings.getExecutionTimeout());
+        }
+        if (settings.hasIdleTimeout()) {
+            process.setIdleTimeout(settings.getIdleTimeout());
         }
     }
 }

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

 
     public ProcessHandler getHandler();
 
-    public Long getStartTime();
+    public long getStartTime();
 
     public void start();
 

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

     }
 
     public ExternalProcessBuilder command(List<String> command, File workingDir, long timeout) {
-        timeout(timeout);
+        idleTimeout(timeout);
 
         return command(command, workingDir);
     }
         return this;
     }
 
+    /**
+     * Sets the absolute number of milliseconds the process is allowed to execute <i>regardless of whether it is idle
+     * or is actively producing output</i>. Setting this limit may be useful to prevent resource-intensive processes
+     * from "running away", tying up the system while they execute for an indefinite amount of time. When this timeout
+     * expires, the process will be killed even if it is not idle.
+     *
+     * @param executionTimeout the number of milliseconds the process is allowed to execute
+     * @return {@code this}
+     */
+    public ExternalProcessBuilder executionTimeout(long executionTimeout) {
+        settings.setExecutionTimeout(executionTimeout);
+
+        return this;
+    }
+
     public ExternalProcessBuilder handler(ProcessHandler handler) {
         settings.setProcessHandler(handler);
 
         return handler(new PluggableProcessHandler(input, output, error));
     }
 
+    /**
+     * Sets the maximum number of milliseconds the process is allowed to remain idle. The exact definition of idle may
+     * vary slightly depending on the {@link OutputHandler} being used, but it generally correlates to time where the
+     * running process is not producing any output to be processed. Concretely, the idle timeout defines the maximum
+     * milliseconds between {@link Watchdog#resetWatchdog()} calls.
+     *
+     * @param idleTimeout the number of milliseconds the running process is allowed to be idle
+     * @return {@code this}
+     */
+    public ExternalProcessBuilder idleTimeout(long idleTimeout) {
+        settings.setIdleTimeout(idleTimeout);
+
+        return this;
+    }
+
     public ExternalProcessBuilder log(Logger logger, Priority priority) {
         return log(logger, priority, null);
     }
         return this;
     }
 
+    /**
+     * Sets the maximum number of milliseconds the process is allowed to remain idle. The exact definition of idle may
+     * vary slightly depending on the {@link OutputHandler} being used, but it generally correlates to time where the
+     * running process is not producing any output to be processed. Concretely, the idle timeout defines the maximum
+     * milliseconds between {@link Watchdog#resetWatchdog()} calls.
+     *
+     * @param timeout the number of milliseconds the running process is allowed to be idle
+     * @return {@code this}
+     * @deprecated Use {@link #idleTimeout(long)} instead. This method will be removed in a future release.
+     */
+    @Deprecated
     public ExternalProcessBuilder timeout(long timeout) {
-        settings.setTimeout(timeout);
-
-        return this;
+        return idleTimeout(timeout);
     }
 
     /**

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

     private static final String OS_NAME = System.getProperty("os.name").toLowerCase();
     private static final ExecutorService POOL;
 
+    private boolean canceled;
     private List<String> command;
+    private Map<String, String> environment;
+    private LatchedRunnable errorPump;
+    private long executionTimeout;
+    private ProcessHandler handler;
+    private long idleTimeout;
+    private LatchedRunnable inputPump;
+    private long lastWatchdogReset;
+    private List<ProcessMonitor> monitors;
+    private LatchedRunnable outputPump;
+    private Process process;
+    private ProcessException processException;
+    private boolean suppressSpecialWindowsBehaviour;
+    private long startTime;
     private File workingDir;
-    private Map<String, String> environment;
-    private ProcessHandler handler;
-    private Process process;
-    private boolean suppressSpecialWindowsBehaviour;
     private boolean useWindowsEncodingWorkaround;
 
-    private List<ProcessMonitor> monitors = new ArrayList<ProcessMonitor>();
-
-    private ProcessException processException;
-
-    private LatchedRunnable outputPump;
-    private LatchedRunnable errorPump;
-    private LatchedRunnable inputPump;
-
-    private long lastWatchdogReset;
-    private long timeout = 60000L;
-    private Long startTime;
-    private boolean canceled;
-
-    public void resetWatchdog() {
-        lastWatchdogReset = System.currentTimeMillis();
-    }
-
-    public long getTimeoutTime() {
-        return lastWatchdogReset + timeout;
-    }
-
-    public boolean isTimedOut() {
-        return getTimeoutTime() < System.currentTimeMillis();
-    }
-
     static {
         final String pooledThreadName = "ExtProcess IO Pump";
         ThreadFactory threadFactory = new ThreadFactory() {
                 return new Thread(r, pooledThreadName);
             }
         };
-        POOL = new ThreadPoolExecutor(6, Integer.MAX_VALUE, 120, TimeUnit.SECONDS,
+        POOL = new ThreadPoolExecutor(6, Integer.MAX_VALUE, 2, TimeUnit.MINUTES,
                 new SynchronousQueue<Runnable>(), threadFactory) {
 
             @Override
     public ExternalProcessImpl(List<String> command, ProcessHandler handler) {
         setCommand(command);
         setHandler(handler);
+
+        idleTimeout = TimeUnit.MINUTES.toMillis(1L);
+        monitors = new ArrayList<ProcessMonitor>();
+        startTime = -1;
     }
 
     /**
         this(ProcessUtils.tokenizeCommand(commandLine), handler);
     }
 
+    public void resetWatchdog() {
+        lastWatchdogReset = System.currentTimeMillis();
+    }
+
+    public long getTimeoutTime() {
+        long timeout = lastWatchdogReset + idleTimeout;
+        if (executionTimeout > 0 && startTime > 0) {
+            timeout = Math.min(timeout, startTime + executionTimeout);
+        }
+        return timeout;
+    }
+
+    public boolean isTimedOut() {
+        return getTimeoutTime() < System.currentTimeMillis();
+    }
+
     protected void setHandler(ProcessHandler handler) {
         this.handler = handler;
     }
     }
 
     /**
-     * @return the time process execution started. null if the process has not yet started.
+     * @return the time process execution started. -1 if the process has not yet started.
      */
-    public Long getStartTime() {
+    public long getStartTime() {
         return this.startTime;
     }
 
         return canceled;
     }
 
+    public void setExecutionTimeout(long executionTimeout) {
+        this.executionTimeout = executionTimeout;
+    }
+
+    public void setIdleTimeout(long idleTimeout) {
+        this.idleTimeout = idleTimeout;
+    }
+
+    @Deprecated
     public void setTimeout(long timeout) {
-        this.timeout = timeout;
+        setIdleTimeout(timeout);
     }
 
     /**

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

     private final List<ProcessMonitor> monitors;
 
     private List<String> command;
+    private long executionTimeout;
+    private long idleTimeout;
     private ProcessHandler processHandler;
     private boolean suppressSpecialWindowsBehaviour;
-    private long timeout;
     private boolean useWindowsEncodingWorkaround;
     private File workingDirectory;
 
         return command;
     }
 
+    public long getExecutionTimeout() {
+        return executionTimeout;
+    }
+
+    public long getIdleTimeout() {
+        return idleTimeout;
+    }
+
     public ProcessHandler getProcessHandler() {
         return processHandler;
     }
 
-    public long getTimeout() {
-        return timeout;
-    }
-
     public File getWorkingDirectory() {
         return workingDirectory;
     }
         return !environment.isEmpty();
     }
 
-    public boolean hasTimeout() {
-        return timeout > 0L;
+    public boolean hasExecutionTimeout() {
+        return executionTimeout > 0L;
+    }
+
+    public boolean hasIdleTimeout() {
+        return idleTimeout > 0L;
     }
 
     public boolean isSuppressSpecialWindowsBehaviour() {
         this.command = command;
     }
 
+    public void setExecutionTimeout(long executionTimeout) {
+        this.executionTimeout = executionTimeout;
+    }
+
+    public void setIdleTimeout(long idleTimeout) {
+        this.idleTimeout = idleTimeout;
+    }
+
     public void setProcessHandler(ProcessHandler processHandler) {
         this.processHandler = processHandler;
     }
         this.suppressSpecialWindowsBehaviour = suppressSpecialWindowsBehaviour;
     }
 
-    public void setTimeout(long timeout) {
-        this.timeout = timeout;
-    }
-
     public void setUseWindowsEncodingWorkaround(boolean useWindowsEncodingWorkaround) {
         this.useWindowsEncodingWorkaround = useWindowsEncodingWorkaround;
     }

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

             // The lineoutputhandler needs to use the same encoding to process the results. Otherwise, encoding errors will occur.
             ExternalProcess process = new ExternalProcessBuilder()
                     .command(echoCmd)
-                    .timeout(2000)
+                    .idleTimeout(2000)
                     .handlers(
                             new LineOutputHandler(encoding) {
                                 @Override

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

+package com.atlassian.utils.process;
+
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.Assert.assertTrue;
+
+public class ExternalProcessImplTest {
+
+    @Test(timeout = 10 * 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))
+                .handlers(new StringOutputHandler()) //Will ensure we don't idle out
+                .build();
+        process.execute();
+
+        ProcessException exception = process.getHandler().getException();
+        assertTrue(exception instanceof ProcessTimeoutException);
+    }
+
+    @Test(timeout = 10 * 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))
+                .handlers(new StringOutputHandler())
+                .build();
+        process.execute();
+
+        ProcessException exception = process.getHandler().getException();
+        assertTrue(exception instanceof ProcessTimeoutException);
+    }
+
+    private boolean isWindows() {
+        return System.getProperty("os.name").toLowerCase().contains("windows");
+    }
+}

src/test/resources/log4j.xml

+<?xml version="1.0" encoding="UTF-8" ?>
+<!DOCTYPE log4j:configuration SYSTEM "log4j.dtd">
+<log4j:configuration xmlns:log4j="http://jakarta.apache.org/log4j/">
+    <appender name="console" class="org.apache.log4j.ConsoleAppender">
+        <layout class="org.apache.log4j.PatternLayout">
+            <param name="ConversionPattern" value="%d %t %p [%c] %m%n"/>
+        </layout>
+    </appender>
+
+    <root>
+        <priority value="warn"/>
+        <appender-ref ref="console"/>
+    </root>
+</log4j:configuration>