Michael Heemskerk avatar Michael Heemskerk committed 3a8d925

STASH-3117 Fix race condition between ExternalProcessImpl.cancel and .finish

When two threads concurrently call cancel and finish, internalCancle could
result in a NPE. This fix ensures that only one thread can call internalCancel
at a time. It also ensures that notifyAfterFinished is only processed once.

Comments (0)

Files changed (1)

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

     private static final Logger LOG = Logger.getLogger(ExternalProcessImpl.class);
     private static final String OS_NAME = System.getProperty("os.name").toLowerCase();
 
-    private AtomicBoolean canceled = new AtomicBoolean(false);
-    private List<String> command;
+    private final AtomicBoolean canceled = new AtomicBoolean(false);
+    private final List<String> command;
     private Map<String, String> environment;
     private LatchedRunnable errorPump;
     private final ExecutorService executorService;
     private long executionTimeout;
-    private AtomicBoolean finished = new AtomicBoolean(false);
+    private final AtomicBoolean finished = new AtomicBoolean(false);
     private ProcessHandler handler;
     private long idleTimeout;
     private LatchedRunnable inputPump;
     private volatile boolean inputPumpInterruptedAfterProcessFinished = false;
     private long lastWatchdogReset;
-    private List<ProcessMonitor> monitors;
+    private final List<ProcessMonitor> monitors;
     private LatchedRunnable outputPump;
     private Process process;
     private ProcessException processException;
      * @param handler the process handler to manage the execution of this process
      */
     public ExternalProcessImpl(ExecutorService executorService, List<String> command, ProcessHandler handler) {
-        setCommand(command);
+        this.command = command;
         setHandler(handler);
 
         this.executorService = executorService;
         idleTimeout = TimeUnit.MINUTES.toMillis(1L);
-        monitors = new ArrayList<ProcessMonitor>();
+        monitors = new CopyOnWriteArrayList<ProcessMonitor>();
         startTime = -1;
     }
 
         this.handler = handler;
     }
 
-    private void setCommand(List<String> command) {
-        this.command = command;
-    }
-
     public void setWorkingDir(File workingDir) {
         this.workingDir = workingDir;
     }
                     "you need to rerun a process.");
         }
         try {
+            this.startTime = System.currentTimeMillis();
             notifyBeforeStart();
-            this.startTime = System.currentTimeMillis();
             this.process = createProcess(command, environment, workingDir);
             setupIOPumps();
         } catch (IOException e) {
      * start() method.
      */
     public void finish() {
-        if (finished.get()) {
+        if (process == null || finished.get()) {
             return;
         }
 
      * @return true if the process has finished.
      */
     public boolean finish(int maxWait) {
-        if (finished.get()) {
+        if (process == null || finished.get()) {
             return true;
         }
 
     }
     
     private void wrapUpProcess() {
+        if (process == null || finished.get()) {
+            return;
+        }
+
         int exitCode = -1;
-        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
-                    }
+        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);
-            }
+        // ensure the process 'after finished' notifications are only triggered once
+        if (finished.compareAndSet(false, true)) {
+            internalCancel(exitCode);
 
             if (processIncomplete && !interrupted) {
                 processException = new ProcessTimeoutException("process timed out");
             }
+
+            handler.complete(exitCode, isCanceled(), processException);
+
+            notifyAfterFinished();
         }
-
-        handler.complete(exitCode, isCanceled(), processException);
-        finished.set(true);
-
-        notifyAfterFinished();
     }
 
     /**
                 try {
                     new WinProcess(process).killRecursively();
                 } catch (Throwable t) {
-                    LOG.error("Failed to kill Windows process; falling back on Process.destroy()", t);
+                    LOG.warn("Failed to kill Windows process; falling back on Process.destroy()", t);
                     process.destroy();
                 }
             } else {
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.