diff --git a/src/main/java/ch/vorburger/exec/AtomicExecuteResultHandler.java b/src/main/java/ch/vorburger/exec/AtomicExecuteResultHandler.java deleted file mode 100644 index 1a3d11e..0000000 --- a/src/main/java/ch/vorburger/exec/AtomicExecuteResultHandler.java +++ /dev/null @@ -1,121 +0,0 @@ -/* - * #%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.time.Duration; -import java.util.Optional; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionException; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import org.apache.commons.exec.ExecuteException; -import org.apache.commons.exec.ExecuteResultHandler; -import org.eclipse.jdt.annotation.Nullable; -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 Issue #108 - */ -public class AtomicExecuteResultHandler implements ExecuteResultHandler { - private static final Logger LOG = LoggerFactory.getLogger(AtomicExecuteResultHandler.class); - - // When reading this code, do not confuse an - // org.apache.commons.exec.ExecuteException and (similarly named) - // an java.util.concurrent.ExecutionException - - private final CompletableFuture holder = new CompletableFuture<>(); - - private void logOnAlreadySet(String methodName, @Nullable ExecuteException e) { - String errorString = methodName + " will throw IllegalStateException, already set: " + holder; - LOG.error(errorString); - throw new IllegalStateException(errorString, e); - } - - @Override - public void onProcessComplete(int exitValue) { - if (!holder.complete(exitValue)) { - logOnAlreadySet("onProcessComplete(" + exitValue + ")", null); - } - } - - @Override - public void onProcessFailed(ExecuteException e) { - if (!holder.completeExceptionally(e)) { - logOnAlreadySet("onProcessFailed(" + e + ")", e); - } - } - - public Optional getExitValue() { - try { - return Optional.ofNullable(holder.getNow(null)); - } catch (CompletionException e) { - // This is thrown when there is no exit value, yet; so: - return Optional.empty(); - } - } - - public Optional getException() { - try { - holder.getNow(null); - return Optional.empty(); - } catch (CompletionException e) { - // This is thrown when there is no exit value, yet; so: - Throwable inner = e.getCause(); - if (inner instanceof ExecuteException) { - return Optional.of((ExecuteException) inner); - } else { - // This should never happen, because we only ever set ExecuteException - throw new IllegalStateException("BUG", inner); - } - } - } - - public void waitFor() throws InterruptedException { - try { - holder.get(); - } catch (InterruptedException e) { - throw e; - } catch (ExecutionException e) { - // see below - } - } - - public void waitFor(Duration timeout) throws InterruptedException { - long timeoutNanos = timeout.toNanos(); - try { - holder.get(timeoutNanos, TimeUnit.NANOSECONDS); - } catch (InterruptedException e) { - throw e; - } catch (ExecutionException | TimeoutException e) { - // Swallow any java.util.concurrent.ExecutionException - // Caused by: org.apache.commons.exec.ExecuteException - // (which we get here after an onProcessFailed()), - // or any java.util.concurrent.TimeoutException; but - // do NOT catch java.util.concurrent.CompletionException. - } - } -} diff --git a/src/main/java/ch/vorburger/exec/ProcessResultHandler.java b/src/main/java/ch/vorburger/exec/CompletableFutureExecuteResultHandler.java similarity index 50% rename from src/main/java/ch/vorburger/exec/ProcessResultHandler.java rename to src/main/java/ch/vorburger/exec/CompletableFutureExecuteResultHandler.java index 0a6b661..bfc98be 100644 --- a/src/main/java/ch/vorburger/exec/ProcessResultHandler.java +++ b/src/main/java/ch/vorburger/exec/CompletableFutureExecuteResultHandler.java @@ -19,32 +19,33 @@ */ package ch.vorburger.exec; +import java.util.concurrent.CompletableFuture; import org.apache.commons.exec.ExecuteException; +import org.apache.commons.exec.ExecuteResultHandler; -/** - * Extends {@link AtomicExecuteResultHandler} with a listener. - */ -class ProcessResultHandler extends AtomicExecuteResultHandler { - private final ManagedProcessListener listener; +class CompletableFutureExecuteResultHandler implements ExecuteResultHandler { + + private final CompletableFuture asyncResult; - ProcessResultHandler(ManagedProcessListener listener) { - if (listener == null) { - //set internal listener - this.listener = new ManagedProcessListenerInternal(); - } else { - this.listener = listener; - } + public CompletableFutureExecuteResultHandler(CompletableFuture asyncResult) { + this.asyncResult = asyncResult; } - @Override + /** + * The asynchronous execution completed. + * + * @param exitValue the exit value of the sub-process + */ public void onProcessComplete(int exitValue) { - super.onProcessComplete(exitValue); - listener.onProcessComplete(exitValue); + asyncResult.complete(exitValue); } - @Override - public void onProcessFailed(ExecuteException processFailedException) { - super.onProcessFailed(processFailedException); - listener.onProcessFailed(processFailedException.getExitValue(), processFailedException); + /** + * The asynchronous execution failed. + * + * @param e the {@code ExecuteException} containing the root cause + */ + public void onProcessFailed(ExecuteException e) { + asyncResult.completeExceptionally(e); } } diff --git a/src/main/java/ch/vorburger/exec/CompositeExecuteResultHandler.java b/src/main/java/ch/vorburger/exec/CompositeExecuteResultHandler.java deleted file mode 100644 index c8939e8..0000000 --- a/src/main/java/ch/vorburger/exec/CompositeExecuteResultHandler.java +++ /dev/null @@ -1,68 +0,0 @@ -/* - * #%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 static java.util.Objects.requireNonNull; - -import java.lang.invoke.MethodHandles; -import java.util.ArrayList; -import java.util.List; -import org.apache.commons.exec.ExecuteException; -import org.apache.commons.exec.ExecuteResultHandler; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -class CompositeExecuteResultHandler extends AtomicExecuteResultHandler { - private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - - private final List handlers; - private final ManagedProcessState managedProcessState; - - public CompositeExecuteResultHandler(ManagedProcessState managedProcessState, - List handlers) { - super(); - this.managedProcessState = requireNonNull(managedProcessState, "managedProcessState can't be null"); - this.handlers = new ArrayList<>(handlers); - } - - @Override - public void onProcessComplete(int exitValue) { - super.onProcessComplete(exitValue); - for (ExecuteResultHandler handler : handlers) { - try { - handler.onProcessComplete(exitValue); - } catch (RuntimeException e) { - logger.error(managedProcessState.getProcLongName() + " process handler failed on processComplete", e); - } - } - } - - @Override - public void onProcessFailed(ExecuteException processFailedException) { - super.onProcessFailed(processFailedException); - for (ExecuteResultHandler handler : handlers) { - try { - handler.onProcessFailed(processFailedException); - } catch (RuntimeException e) { - logger.error(managedProcessState.getProcLongName() + " process handler failed on processComplete", e); - } - } - } -} diff --git a/src/main/java/ch/vorburger/exec/LoggingExecuteResultHandler.java b/src/main/java/ch/vorburger/exec/LoggingExecuteResultHandler.java deleted file mode 100644 index e31f360..0000000 --- a/src/main/java/ch/vorburger/exec/LoggingExecuteResultHandler.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * #%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 static java.util.Objects.requireNonNull; - -import java.lang.invoke.MethodHandles; -import org.apache.commons.exec.ExecuteException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * Extends our {@link AtomicExecuteResultHandler} with logging and notify state to initializing class. - */ -public class LoggingExecuteResultHandler extends AtomicExecuteResultHandler { - private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - - private final ManagedProcessState managedProcessState; - - public LoggingExecuteResultHandler(ManagedProcessState managedProcessState) { - super(); - this.managedProcessState = requireNonNull(managedProcessState, "managedProcessState can't be null"); - } - - @Override - public void onProcessComplete(int exitValue) { - super.onProcessComplete(exitValue); - logger.info(managedProcessState.getProcLongName() + " just exited, with value " + exitValue); - managedProcessState.notifyProcessHalted(); - } - - @Override - public void onProcessFailed(ExecuteException e) { - super.onProcessFailed(e); - if (!managedProcessState.watchDogKilledProcess()) { - logger.error(managedProcessState.getProcLongName() + " failed unexpectedly", e); - } - managedProcessState.notifyProcessHalted(); - } -} diff --git a/src/main/java/ch/vorburger/exec/ManagedProcess.java b/src/main/java/ch/vorburger/exec/ManagedProcess.java index 42f2962..7acc0ae 100644 --- a/src/main/java/ch/vorburger/exec/ManagedProcess.java +++ b/src/main/java/ch/vorburger/exec/ManagedProcess.java @@ -35,8 +35,12 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import java.util.function.Function; import org.apache.commons.exec.CommandLine; +import org.apache.commons.exec.ExecuteException; import org.apache.commons.exec.ExecuteWatchdog; import org.apache.commons.exec.Executor; import org.apache.commons.exec.ProcessDestroyer; @@ -76,7 +80,7 @@ public class ManagedProcess implements ManagedProcessState { private final StopCheckExecuteWatchdog watchDog = new StopCheckExecuteWatchdog(ExecuteWatchdog.INFINITE_TIMEOUT); private final ProcessDestroyer shutdownHookProcessDestroyer = new LoggingShutdownHookProcessDestroyer(); private final Map environment; - private final CompositeExecuteResultHandler resultHandler; + private final CompletableFuture asyncResult; private final @Nullable InputStream input; private final boolean destroyOnShutdown; private final int consoleBufferMaxLines; @@ -129,8 +133,25 @@ public class ManagedProcess implements ManagedProcessState { this.destroyOnShutdown = destroyOnShutdown; this.consoleBufferMaxLines = consoleBufferMaxLines; this.outputStreamLogDispatcher = outputStreamLogDispatcher; - this.resultHandler = new CompositeExecuteResultHandler(this, - Arrays.asList(new LoggingExecuteResultHandler(this), new ProcessResultHandler(listener))); + this.asyncResult = new CompletableFuture<>(); + this.asyncResult.handle((result, e) -> + { + if (e == null) { + logger.info(this.getProcLongName() + " just exited, with value " + result); + listener.onProcessComplete(result); + } else { + logger.error(this.getProcLongName() + " failed unexpectedly", e); + if (e instanceof ExecuteException) { + ExecuteException ee = (ExecuteException) e; + listener.onProcessFailed(ee.getExitValue(), ee); + } // TODO handle non-ExecuteException cases gracefully + } + if (e != null && !(e instanceof CancellationException)) { + this.notifyProcessHalted(); + } + return null; + } + ); this.stdoutOS = new MultiOutputStream(); this.stderrOS = new MultiOutputStream(); for (OutputStream stdOut : stdOuts) { @@ -201,13 +222,13 @@ public File getExecutableFile() { protected synchronized void startExecute() throws ManagedProcessException { try { - executor.execute(commandLine, environment, resultHandler); + executor.execute(commandLine, environment, new CompletableFutureExecuteResultHandler(asyncResult)); } catch (IOException e) { throw new ManagedProcessException("Launch failed: " + commandLine, e); } // We now must give the system a say 100ms chance to run the background - // thread now, otherwise the resultHandler in checkResult() won't work. + // thread now, otherwise the asyncResult in checkResult() won't work. // // This is admittedly not ideal, but to do better would require significant // changes to DefaultExecutor, so that its execute() would "fail fast" and @@ -312,14 +333,26 @@ protected ManagedProcessException handleInterruptedException(InterruptedExceptio return new ManagedProcessException(message, e); } + protected ManagedProcessException handleException(Exception e) + throws ManagedProcessException { + // TODO Not sure how to best handle this... opinions welcome (see also below) + final String message = "Huh?! Exception should normally never happen here..." + + getProcLongName(); + logger.error(message, e); + return new ManagedProcessException(message, e); + } + + // TODO we could add this as a closure on the CompletableFuture instead of checking protected void checkResult() throws ManagedProcessException { - Optional opt = resultHandler.getException(); - if (opt.isPresent()) { + if (asyncResult.isCompletedExceptionally()) { // We already terminated (or never started) - // 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()); + try { + asyncResult.get(); // just called to throw the exception + } catch (Exception e) { + logger.error(getProcLongName() + " failed", e); + throw new ManagedProcessException(getProcLongName() + " failed with Exception: " + getLastConsoleLines(), + e); + } } } @@ -336,8 +369,9 @@ public void destroy() throws ManagedProcessException { // Note: If destroy() is ever giving any trouble, the // org.openqa.selenium.os.ProcessUtils may be of interest. if (!isAlive) { + asyncResult.cancel(false); throw new ManagedProcessException(getProcLongName() - + " was already stopped (or never started)"); + + " was already stopped (or never started)"); } if (logger.isDebugEnabled()) { logger.debug("Going to destroy {}", getProcLongName()); @@ -347,9 +381,11 @@ public void destroy() throws ManagedProcessException { try { // Safer to waitFor() after destroy() - resultHandler.waitFor(); + asyncResult.get(); } catch (InterruptedException e) { throw handleInterruptedException(e); + } catch (Exception e) { + throw handleException(e); } if (logger.isInfoEnabled()) { @@ -368,7 +404,7 @@ public void destroy() throws ManagedProcessException { */ @Override public boolean isAlive() { - // NOPE: return !resultHandler.hasResult(); + // NOPE: return !asyncResult.hasResult(); return isAlive; } @@ -395,17 +431,11 @@ public void notifyProcessHalted() { */ @Override public int exitValue() throws ManagedProcessException { - Optional optExit = resultHandler.getExitValue(); - if (optExit.isPresent()) { - return optExit.get(); - } else { - Optional 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()); + try { + return asyncResult.get(); + } catch (Exception e) { + throw new ManagedProcessException("No Exit Value, but an exception, is available for " + + getProcLongName(), e); } } @@ -452,9 +482,9 @@ protected int waitForExitMaxMsWithoutLog(long maxWaitUntilReturningInMS) assertWaitForIsValid(); try { if (maxWaitUntilReturningInMS != -1) { - resultHandler.waitFor(Duration.ofMillis(maxWaitUntilReturningInMS)); + asyncResult.get(maxWaitUntilReturningInMS, TimeUnit.MILLISECONDS); } else { - resultHandler.waitFor(); + asyncResult.get(); } // We will reach here in 4 cases: @@ -469,7 +499,7 @@ protected int waitForExitMaxMsWithoutLog(long maxWaitUntilReturningInMS) checkResult(); // This returns the exit value - iff we have one - Optional exit = resultHandler.getExitValue(); + Optional exit = Optional.ofNullable(asyncResult.getNow(null)); if (exit.isPresent()) { return exit.get(); } @@ -481,6 +511,8 @@ protected int waitForExitMaxMsWithoutLog(long maxWaitUntilReturningInMS) } } catch (InterruptedException e) { throw handleInterruptedException(e); + } catch (Exception e) { + throw handleException(e); } } @@ -505,7 +537,7 @@ public ManagedProcess waitForExitMaxMsOrDestroy(long maxWaitUntilDestroyTimeout) } protected void assertWaitForIsValid() throws ManagedProcessException { - if (!watchDog.isStopped() && !isAlive() && !resultHandler.getExitValue().isPresent()) { + if (!watchDog.isStopped() && !isAlive() && !asyncResult.isDone()) { throw new ManagedProcessException("Asked to waitFor " + getProcLongName() + ", but it was never even start()'ed!"); }