Replace DefaultExecuteResultHandler with AtomicExecuteResultHandler (…

…re. #108)

see #108

src/main/java/ch/vorburger/exec/AtomicExecuteResultHandler.java

@@ -0,0 +1,106 @@
+/*
+ * #%L
+ * ch.vorburger.exec
+ * %%
+ * Copyright (C) 2012 - 2023 Michael Vorburger
+ * %%
+ * Licensed 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.
+ * #L%
+ */
+package ch.vorburger.exec;
+
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.commons.exec.ExecuteException;
+import org.apache.commons.exec.ExecuteResultHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An {@link ExecuteResultHandler} which uses a single
+ * {@link java.util.concurrent.atomic.AtomicReference}
+ * instead of three separate volatile fields like the
+ * original {@link org.apache.commons.exec.DefaultExecuteResultHandler} did.
+ *
+ * @see <a href="https://github.com/vorburger/ch.vorburger.exec/issues/108">Issue #108</a>
+ */
+public class AtomicExecuteResultHandler implements ExecuteResultHandler {
+    private static final Logger LOG = LoggerFactory.getLogger(AtomicExecuteResultHandler.class);
+
+    /** The interval polling the result */
+    private static final int SLEEP_TIME_MS = 50;
+
+    private static class Holder {
+        /** The exit value of the finished process */
+        private final Integer exitValue;
+
+        /** Any offending exception */
+        private final ExecuteException exception;
+
+        Holder(int exitValue) {
+            this.exitValue = exitValue;
+            this.exception = null;
+        }
+
+        Holder(ExecuteException e) {
+            this.exception = e;
+            this.exitValue = null;
+        }
+
+        @Override
+        public String toString() {
+            return "exitValue=" + exitValue + ", exception=" + exception;
+        }
+    }
+
+    private final AtomicReference<Holder> holder = new AtomicReference<>();
+
+    @Override
+    public void onProcessComplete(int exitValue) {
+        if (!holder.compareAndSet(null, new Holder(exitValue))) {
+            LOG.error("onProcessComplete({}) will throw IllegalStateException, already set: {}", exitValue, holder);
+            throw new IllegalStateException("Result already set");
+        }
+    }
+
+    @Override
+    public void onProcessFailed(ExecuteException e) {
+        if (!holder.compareAndSet(null, new Holder(e))) {
+            LOG.error("onProcessFailed({}) will throw IllegalStateException, already set: {}", e, holder);
+            throw new IllegalStateException("Result already set");
+        }
+    }
+
+    public Optional<Integer> getExitValue() {
+        Holder current = holder.get();
+        return current != null ? Optional.ofNullable(current.exitValue) : Optional.empty();
+    }
+
+    public Optional<Exception> getException() {
+        Holder current = holder.get();
+        return current != null ? Optional.ofNullable(current.exception) : Optional.empty();
+    }
+
+    public void waitFor() throws InterruptedException {
+        while (holder.get() == null) {
+            Thread.sleep(SLEEP_TIME_MS);
+        }
+    }
+
+    public void waitFor(long timeoutInMS) throws InterruptedException {
+        final long until = System.currentTimeMillis() + timeoutInMS;
+        while (holder.get() == null && System.currentTimeMillis() < until) {
+            Thread.sleep(SLEEP_TIME_MS);
+        }
+    }
+}

src/main/java/ch/vorburger/exec/CompositeExecuteResultHandler.java

@@ -24,13 +24,12 @@
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
 import java.util.List;
-import org.apache.commons.exec.DefaultExecuteResultHandler;
 import org.apache.commons.exec.ExecuteException;
 import org.apache.commons.exec.ExecuteResultHandler;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-class CompositeExecuteResultHandler extends DefaultExecuteResultHandler {
+class CompositeExecuteResultHandler extends AtomicExecuteResultHandler {
     private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
     private final List<? extends ExecuteResultHandler> handlers;

src/main/java/ch/vorburger/exec/LoggingExecuteResultHandler.java

@@ -22,15 +22,14 @@
 import static java.util.Objects.requireNonNull;
 
 import java.lang.invoke.MethodHandles;
-import org.apache.commons.exec.DefaultExecuteResultHandler;
 import org.apache.commons.exec.ExecuteException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * Extends Commons Exec's {@link DefaultExecuteResultHandler} with logging and notify state to initializing class.
+ * Extends our {@link AtomicExecuteResultHandler} with logging and notify state to initializing class.
  */
-public class LoggingExecuteResultHandler extends DefaultExecuteResultHandler {
+public class LoggingExecuteResultHandler extends AtomicExecuteResultHandler {
     private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
     private final ManagedProcessState managedProcessState;

src/main/java/ch/vorburger/exec/ManagedProcess.java

@@ -31,9 +31,9 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import org.apache.commons.exec.CommandLine;
 import org.apache.commons.exec.DefaultExecutor;
-import org.apache.commons.exec.ExecuteException;
 import org.apache.commons.exec.ExecuteWatchdog;
 import org.apache.commons.exec.Executor;
 import org.apache.commons.exec.ProcessDestroyer;
@@ -303,14 +303,13 @@ protected ManagedProcessException handleInterruptedException(InterruptedExceptio
     }
 
     protected void checkResult() throws ManagedProcessException {
-        if (resultHandler.hasResult()) {
+        Optional<Exception> opt = resultHandler.getException();
+        if (opt.isPresent()) {
             // We already terminated (or never started)
-            ExecuteException e = resultHandler.getException();
-            if (e != null) {
-                logger.error(getProcLongName() + " failed");
-                throw new ManagedProcessException(getProcLongName() + " failed, exitValue="
-                        + exitValue() + getLastConsoleLines(), e);
-            }
+            // Nota bene: Do NOT getExitValue() - it's either/or!
+            logger.error(getProcLongName() + " failed", opt.get());
+            throw new ManagedProcessException(getProcLongName() + " failed with Exception: " + getLastConsoleLines(),
+                    opt.get());
         }
     }
 
@@ -324,11 +323,8 @@ protected void checkResult() throws ManagedProcessException {
      */
     @Override
     public void destroy() throws ManagedProcessException {
-        //
-        // if destroy() is ever giving any trouble, the org.openqa.selenium.os.ProcessUtils may be
-        // of
-        // interest
-        //
+        // Note: If destroy() is ever giving any trouble, the
+        // org.openqa.selenium.os.ProcessUtils may be of interest.
         if (!isAlive) {
             throw new ManagedProcessException(getProcLongName()
                     + " was already stopped (or never started)");
@@ -384,15 +380,22 @@ public void notifyProcessHalted() {
      * @return the exit value of the subprocess represented by this <code>Process</code> object. by
      *         convention, the value <code>0</code> indicates normal termination.
      * @exception ManagedProcessException if the subprocess represented by this
-     *                <code>ManagedProcess</code> object has not yet terminated.
+     *                <code>ManagedProcess</code> object has not yet terminated,
+     *                or has terminated without an exit value.
      */
     @Override
     public int exitValue() throws ManagedProcessException {
-        try {
-            return resultHandler.getExitValue();
-        } catch (IllegalStateException e) {
-            throw new ManagedProcessException("Exit Value not (yet) available for "
-                    + getProcLongName(), e);
+        Optional<Integer> optExit = resultHandler.getExitValue();
+        if (optExit.isPresent()) {
+            return optExit.get();
+        } else {
+            Optional<Exception> optError = resultHandler.getException();
+            if (optError.isPresent()) {
+                throw new ManagedProcessException("No Exit Value, but an exception, is available for "
+                        + getProcLongName(), optError.get());
+            }
+            throw new ManagedProcessException("Neither Exit Value nor an Exception are available (yet) for "
+                    + getProcLongName());
         }
     }
 
@@ -434,12 +437,12 @@ public int waitForExitMaxMs(long maxWaitUntilReturning) throws ManagedProcessExc
         return waitForExitMaxMsWithoutLog(maxWaitUntilReturning);
     }
 
-    protected int waitForExitMaxMsWithoutLog(long maxWaitUntilReturning)
+    protected int waitForExitMaxMsWithoutLog(long maxWaitUntilReturningInMS)
             throws ManagedProcessException {
         assertWaitForIsValid();
         try {
-            if (maxWaitUntilReturning != -1) {
-                resultHandler.waitFor(maxWaitUntilReturning);
+            if (maxWaitUntilReturningInMS != -1) {
+                resultHandler.waitFor(maxWaitUntilReturningInMS);
                 checkResult();
                 if (!isAlive()) {
                     return exitValue();
@@ -475,7 +478,7 @@ public ManagedProcess waitForExitMaxMsOrDestroy(long maxWaitUntilDestroyTimeout)
     }
 
     protected void assertWaitForIsValid() throws ManagedProcessException {
-        if (!watchDog.isStopped() && !isAlive() && !resultHandler.hasResult()) {
+        if (!watchDog.isStopped() && !isAlive() && !resultHandler.getExitValue().isPresent()) {
             throw new ManagedProcessException("Asked to waitFor " + getProcLongName()
                     + ", but it was never even start()'ed!");
         }

src/main/java/ch/vorburger/exec/ProcessResultHandler.java

@@ -19,16 +19,15 @@
  */
 package ch.vorburger.exec;
 
-import org.apache.commons.exec.DefaultExecuteResultHandler;
 import org.apache.commons.exec.ExecuteException;
 
-class ProcessResultHandler extends DefaultExecuteResultHandler {
+/**
+ * Extends {@link AtomicExecuteResultHandler} with a listener.
+ */
+class ProcessResultHandler extends AtomicExecuteResultHandler {
     private final ManagedProcessListener listener;
 
-    /**
-     * extends <CODE>DefaultExecuteResultHandler</CODE> used for asynchronous process handling.
-     */
-    public ProcessResultHandler(ManagedProcessListener listener) {
+    ProcessResultHandler(ManagedProcessListener listener) {
         if (listener == null) {
             //set internal listener
             this.listener = new ManagedProcessListenerInternal();