diff --git a/java/maven_deps.bzl b/java/maven_deps.bzl index 447a4b47c744d..871c7eb1b6c8a 100644 --- a/java/maven_deps.bzl +++ b/java/maven_deps.bzl @@ -72,7 +72,6 @@ def selenium_java_deps(): "it.ozimov:embedded-redis:0.7.3", "net.bytebuddy:byte-buddy:1.14.5", "net.sourceforge.htmlunit:htmlunit-core-js:2.70.0", - "org.apache.commons:commons-exec:1.3", "org.apache.logging.log4j:log4j-core:2.20.0", "org.assertj:assertj-core:3.24.2", "org.bouncycastle:bcpkix-jdk15on:1.70", diff --git a/java/maven_install.json b/java/maven_install.json index 8ba656db743fb..7b1fac95e50b2 100644 --- a/java/maven_install.json +++ b/java/maven_install.json @@ -1,7 +1,7 @@ { "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL", - "__INPUT_ARTIFACTS_HASH": -486878249, - "__RESOLVED_ARTIFACTS_HASH": 1235473565, + "__INPUT_ARTIFACTS_HASH": -521743349, + "__RESOLVED_ARTIFACTS_HASH": -882481443, "artifacts": { "com.beust:jcommander": { "shasums": { @@ -545,13 +545,6 @@ }, "version": "6.5.0" }, - "org.apache.commons:commons-exec": { - "shasums": { - "jar": "cb49812dc1bfb0ea4f20f398bcae1a88c6406e213e67f7524fb10d4f8ad9347b", - "sources": "c121d8e70010092bafc56f358e7259ac484653db595aafea1e67a040f51aea66" - }, - "version": "1.3" - }, "org.apache.commons:commons-lang3": { "shasums": { "jar": "d919d904486c037f8d193412da0c92e22a9fa24230b9d67a57855c5c31c7e94e", @@ -2119,12 +2112,6 @@ "org.apache.bcel.verifier.statics", "org.apache.bcel.verifier.structurals" ], - "org.apache.commons:commons-exec": [ - "org.apache.commons.exec", - "org.apache.commons.exec.environment", - "org.apache.commons.exec.launcher", - "org.apache.commons.exec.util" - ], "org.apache.commons:commons-lang3": [ "org.apache.commons.lang3", "org.apache.commons.lang3.arch", @@ -3202,8 +3189,6 @@ "net.sourceforge.htmlunit:htmlunit-core-js:jar:sources", "org.apache.bcel:bcel", "org.apache.bcel:bcel:jar:sources", - "org.apache.commons:commons-exec", - "org.apache.commons:commons-exec:jar:sources", "org.apache.commons:commons-lang3", "org.apache.commons:commons-lang3:jar:sources", "org.apache.commons:commons-text", @@ -3455,8 +3440,6 @@ "net.sourceforge.htmlunit:htmlunit-core-js:jar:sources", "org.apache.bcel:bcel", "org.apache.bcel:bcel:jar:sources", - "org.apache.commons:commons-exec", - "org.apache.commons:commons-exec:jar:sources", "org.apache.commons:commons-lang3", "org.apache.commons:commons-lang3:jar:sources", "org.apache.commons:commons-text", diff --git a/java/src/org/openqa/selenium/manager/SeleniumManager.java b/java/src/org/openqa/selenium/manager/SeleniumManager.java index f6d8b02c35252..1ec923a003c83 100644 --- a/java/src/org/openqa/selenium/manager/SeleniumManager.java +++ b/java/src/org/openqa/selenium/manager/SeleniumManager.java @@ -28,6 +28,7 @@ import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -43,7 +44,7 @@ import org.openqa.selenium.json.Json; import org.openqa.selenium.json.JsonException; import org.openqa.selenium.manager.SeleniumManagerOutput.Result; -import org.openqa.selenium.os.CommandLine; +import org.openqa.selenium.os.OsProcess; /** * This implementation is still in beta, and may change. @@ -112,15 +113,14 @@ private static Result runCommand(Path binary, List arguments) { String output; int code; try { - CommandLine command = - new CommandLine(binary.toAbsolutePath().toString(), arguments.toArray(new String[0])); - command.executeAsync(); - command.waitFor(); - if (command.isRunning()) { - LOG.warning("Selenium Manager did not exit"); + OsProcess process = + OsProcess.builder().command(binary.toAbsolutePath().toString(), arguments).start(); + if (!process.waitFor(Duration.ofHours(1))) { + LOG.warning("Selenium Manager did not exit, shutting it down"); + process.shutdown(); } - code = command.getExitCode(); - output = command.getStdOut(); + code = process.exitValue(); + output = process.getOutput(); } catch (Exception e) { throw new WebDriverException("Failed to run command: " + arguments, e); } diff --git a/java/src/org/openqa/selenium/os/BUILD.bazel b/java/src/org/openqa/selenium/os/BUILD.bazel index 7338b9f50ec9c..96516598a787d 100644 --- a/java/src/org/openqa/selenium/os/BUILD.bazel +++ b/java/src/org/openqa/selenium/os/BUILD.bazel @@ -11,7 +11,6 @@ java_export( deps = [ "//java/src/org/openqa/selenium:core", "//java/src/org/openqa/selenium/io", - artifact("org.apache.commons:commons-exec"), artifact("com.google.guava:guava"), ], ) diff --git a/java/src/org/openqa/selenium/os/CommandLine.java b/java/src/org/openqa/selenium/os/CommandLine.java deleted file mode 100644 index 56520b2c93bc3..0000000000000 --- a/java/src/org/openqa/selenium/os/CommandLine.java +++ /dev/null @@ -1,174 +0,0 @@ -// Licensed to the Software Freedom Conservancy (SFC) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The SFC licenses this file -// to you 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. - -package org.openqa.selenium.os; - -import static org.openqa.selenium.Platform.MAC; -import static org.openqa.selenium.Platform.WINDOWS; - -import java.io.File; -import java.io.OutputStream; -import java.util.Map; -import java.util.concurrent.TimeUnit; -import org.openqa.selenium.Platform; -import org.openqa.selenium.WebDriverException; - -public class CommandLine { - - private final OsProcess process; - - public CommandLine(String executable, String... args) { - process = new OsProcess(executable, args); - } - - /** - * Adds the specified environment variables. - * - * @param environment the variables to add - * @throws IllegalArgumentException if any value given is null (unsupported) - */ - public void setEnvironmentVariables(Map environment) { - for (Map.Entry entry : environment.entrySet()) { - setEnvironmentVariable(entry.getKey(), entry.getValue()); - } - } - - /** - * Adds the specified environment variable. - * - * @param name the name of the environment variable - * @param value the value of the environment variable - * @throws IllegalArgumentException if the value given is null (unsupported) - */ - public void setEnvironmentVariable(String name, String value) { - process.setEnvironmentVariable(name, value); - } - - public void setDynamicLibraryPath(String newLibraryPath) { - // because on Windows, it is null according to SingleBrowserLocator.computeLibraryPath() - if (newLibraryPath != null) { - setEnvironmentVariable(getLibraryPathPropertyName(), newLibraryPath); - } - } - - public void updateDynamicLibraryPath(String extraPath) { - if (extraPath != null) { - String existing = System.getenv(getLibraryPathPropertyName()); - String ldPath = existing != null ? existing + File.pathSeparator + extraPath : extraPath; - setEnvironmentVariable(getLibraryPathPropertyName(), ldPath); - } - } - - /** - * @return The platform specific env property name which contains the library path. - */ - public static String getLibraryPathPropertyName() { - Platform current = Platform.getCurrent(); - - if (current.is(WINDOWS)) { - return "PATH"; - - } else if (current.is(MAC)) { - return "DYLD_LIBRARY_PATH"; - - } else { - return "LD_LIBRARY_PATH"; - } - } - - public void executeAsync() { - process.executeAsync(); - } - - public void execute() { - executeAsync(); - waitFor(); - } - - public boolean waitForProcessStarted(long duration, TimeUnit unit) { - return process.waitForProcessStarted(duration, unit); - } - - public void waitFor() { - try { - process.waitFor(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new WebDriverException(e); - } - } - - public void waitFor(long timeout) { - try { - process.waitFor(timeout); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new WebDriverException(e); - } - } - - public boolean isSuccessful() { - return 0 == getExitCode(); - } - - public int getExitCode() { - return process.getExitCode(); - } - - public String getStdOut() { - return process.getStdOut(); - } - - /** - * Destroy the current command. - * - * @return The exit code of the command. - */ - public int destroy() { - return process.destroy(); - } - - /** - * Check whether the current command is still executing. - * - * @return true if the current command is still executing, false otherwise - */ - public boolean isRunning() { - return process.isRunning(); - } - - public void setInput(String allInput) { - process.setInput(allInput); - } - - public void setWorkingDirectory(String workingDirectory) { - process.setWorkingDirectory(new File(workingDirectory)); - } - - @Override - public String toString() { - return process.toString(); - } - - public void copyOutputTo(OutputStream out) { - process.copyOutputTo(out); - } - - public void checkForError() { - process.checkForError(); - } -} diff --git a/java/src/org/openqa/selenium/os/OsProcess.java b/java/src/org/openqa/selenium/os/OsProcess.java index a2786a8f1bd31..5ea8c5671d380 100644 --- a/java/src/org/openqa/selenium/os/OsProcess.java +++ b/java/src/org/openqa/selenium/os/OsProcess.java @@ -17,267 +17,270 @@ package org.openqa.selenium.os; -import static java.util.Collections.unmodifiableMap; import static java.util.concurrent.TimeUnit.SECONDS; -import static org.openqa.selenium.Platform.WINDOWS; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStream; -import java.nio.charset.Charset; -import java.util.HashMap; +import java.io.UncheckedIOException; +import java.time.Duration; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; -import org.apache.commons.exec.DaemonExecutor; -import org.apache.commons.exec.DefaultExecuteResultHandler; -import org.apache.commons.exec.ExecuteWatchdog; -import org.apache.commons.exec.Executor; -import org.apache.commons.exec.PumpStreamHandler; -import org.openqa.selenium.Platform; -import org.openqa.selenium.TimeoutException; -import org.openqa.selenium.WebDriverException; import org.openqa.selenium.internal.Require; import org.openqa.selenium.io.CircularOutputStream; import org.openqa.selenium.io.MultiOutputStream; -class OsProcess { +public class OsProcess { private static final Logger LOG = Logger.getLogger(OsProcess.class.getName()); - private final CircularOutputStream inputOut = new CircularOutputStream(32768); - private volatile String allInput; - private final DefaultExecuteResultHandler handler = new DefaultExecuteResultHandler(); - private final Executor executor = new DaemonExecutor(); + public static class Builder { - private volatile OutputStream drainTo; - private SeleniumWatchDog executeWatchdog = new SeleniumWatchDog(ExecuteWatchdog.INFINITE_TIMEOUT); - private PumpStreamHandler streamHandler; + private ProcessBuilder builder; + private OutputStream copyOutputTo; + private int bufferSize = 32768; - private final org.apache.commons.exec.CommandLine cl; - private final Map env = new ConcurrentHashMap<>(); + Builder() { + this.builder = new ProcessBuilder(); + } - public OsProcess(String executable, String... args) { - String actualExe = new ExecutableFinder().find(executable); - Require.state("Actual executable", actualExe) - .nonNull("Unable to find executable for: %s", executable); - cl = new org.apache.commons.exec.CommandLine(actualExe); - cl.addArguments(args, false); - } + /** + * Set the executable command to start the process, this consists of the executable and the + * arguments. + * + * @param executable the executable to build the command + * @param arguments the arguments to build the command + * @return this instance to continue building + */ + public Builder command(String executable, List arguments) { + List command = new ArrayList<>(arguments.size() + 1); + command.add(executable); + command.addAll(arguments); + builder.command(command); + + return this; + } - public void setEnvironmentVariable(String name, String value) { - if (name == null) { - throw new IllegalArgumentException("Cannot have a null environment variable name!"); + /** + * Set the executable command to start the process, this consists of the executable and the + * arguments. + * + * @param command the executable, followed by the arguments + * @return this instance to continue building + */ + public Builder command(List command) { + builder.command(command); + + return this; } - if (value == null) { - throw new IllegalArgumentException( - "Cannot have a null value for environment variable " + name); + + /** + * Set the executable command to start the process, this consists of the executable and the + * arguments. + * + * @param command the executable, followed by the arguments + * @return this instance to continue building + */ + public Builder command(String... command) { + builder.command(command); + + return this; } - env.put(name, value); - } - public Map getEnvironment() { - return unmodifiableMap(new HashMap<>(env)); - } + /** + * Get the executable command to start the process, this consists of the binary and the + * arguments. + * + * @return an editable list, changes to it will update the command executed. + */ + public List command() { + return Collections.unmodifiableList(builder.command()); + } - private Map getMergedEnv() { - HashMap newEnv = new HashMap<>(System.getenv()); - newEnv.putAll(env); - return newEnv; - } + /** + * Set one environment variable of the process to start, will replace the old value if exists. + * + * @return this instance to continue building + */ + public Builder environment(String name, String value) { + Require.argument("name", name).nonNull(); + Require.argument("value", value).nonNull(); - private ByteArrayInputStream getInputStream() { - return allInput != null - ? new ByteArrayInputStream(allInput.getBytes(Charset.defaultCharset())) - : null; - } + builder.environment().put(name, value); - public void executeAsync() { - try { - final OutputStream outputStream = getOutputStream(); - executeWatchdog.reset(); - executor.setWatchdog(executeWatchdog); - streamHandler = new PumpStreamHandler(outputStream, outputStream, getInputStream()); - executor.setStreamHandler(streamHandler); - executor.execute(cl, getMergedEnv(), handler); - } catch (IOException e) { - throw new WebDriverException(e); + return this; } - } - public boolean waitForProcessStarted(long duration, TimeUnit unit) { - return executeWatchdog.waitForProcessStarted(duration, unit); - } + /** + * Get the environment variables of the process to start. + * + * @return an editable map, changes to it will update the environment variables of the command + * executed. + */ + public Map environment() { + return builder.environment(); + } - private OutputStream getOutputStream() { - return drainTo == null ? inputOut : new MultiOutputStream(inputOut, drainTo); - } + /** + * Get the working directory of the process to start, maybe null. + * + * @return the working directory + */ + public File directory() { + return builder.directory(); + } - public int destroy() { - SeleniumWatchDog watchdog = executeWatchdog; + /** + * Set the working directory of the process to start. + * + * @param directory the path to the directory + * @return this instance to continue building + */ + public Builder directory(String directory) { + return directory(new File(directory)); + } - if (watchdog.waitForProcessStarted(2, TimeUnit.MINUTES)) { - // I literally have no idea why we don't try and kill the process nicely on Windows. If you - // do, - // answers on the back of a postcard to SeleniumHQ, please. - if (!Platform.getCurrent().is(WINDOWS)) { - watchdog.destroyProcess(); - watchdog.waitForTerminationAfterDestroy(2, SECONDS); - } + /** + * Set the working directory of the process to start. + * + * @param directory the path to the directory + * @return this instance to continue building + */ + public Builder directory(File directory) { + builder.directory(directory); - if (isRunning()) { - watchdog.destroyHarder(); - watchdog.waitForTerminationAfterDestroy(1, SECONDS); - } - } else { - LOG.warning("Tried to destory a process which never started."); + return this; } - // Make a best effort to drain the streams. - if (streamHandler != null) { - // Stop trying to read the output stream so that we don't race with the stream being closed - // when the process is destroyed. - streamHandler.setStopTimeout(2000); - try { - streamHandler.stop(); - } catch (IOException e) { - // Ignore and destroy the process anyway. - LOG.log( - Level.INFO, - "Unable to drain process streams. Ignoring but the exception being swallowed follows.", - e); - } + /** + * Where to copy the combined stdout and stderr output to, {@code OsProcess#getOutput} is still + * working when called. + * + * @param stream where to copy the combined output to + * @return this instance to continue building + */ + public Builder copyOutputTo(OutputStream stream) { + copyOutputTo = stream; + + return this; } - if (!isRunning()) { - return getExitCode(); - } + /** + * The number of bytes to buffer for {@code OsProcess#getOutput} calls. + * + * @param toKeep the number of bytes, default is 4096 + * @return this instance to continue building + */ + public Builder bufferSize(int toKeep) { + bufferSize = toKeep; - LOG.severe(String.format("Unable to kill process %s", watchdog.process)); - int exitCode = -1; - executor.setExitValue(exitCode); - return exitCode; - } + return this; + } - public void waitFor() throws InterruptedException { - handler.waitFor(); - } + public OsProcess start() throws UncheckedIOException { + // redirect the stderr to stdout + builder.redirectErrorStream(true); - public void waitFor(long timeout) throws InterruptedException { - long until = System.currentTimeMillis() + timeout; - boolean timedOut = true; - while (System.currentTimeMillis() < until) { - if (Thread.interrupted()) { - throw new InterruptedException(); - } - if (handler.hasResult()) { - timedOut = false; - break; + Process process; + try { + process = builder.start(); + } catch (IOException ex) { + throw new UncheckedIOException(ex); } - Thread.sleep(50); - } - if (timedOut) { - throw new TimeoutException( - String.format("Process timed out after waiting for %d ms.", timeout)); - } - // Wait until syserr and sysout have been read - } - - public boolean isRunning() { - return !handler.hasResult(); - } + CircularOutputStream circular; + try { + circular = new CircularOutputStream(bufferSize); + + new Thread( + () -> { + InputStream input = process.getInputStream(); + // use the CircularOutputStream as mandatory, we know it will never raise a + // IOException + OutputStream output = new MultiOutputStream(circular, copyOutputTo); + while (process.isAlive()) { + try { + // we must read the output to ensure the process will not lock up + input.transferTo(output); + } catch (IOException ex) { + LOG.log( + Level.WARNING, + "failed to copy the output of process " + process.pid(), + ex); + } + } + }) + .start(); + } catch (Throwable t) { + // ensure we do not leak a process in case of failures + try { + process.destroyForcibly(); + } catch (Throwable t2) { + t.addSuppressed(t2); + } + throw t; + } - public int getExitCode() { - if (isRunning()) { - throw new IllegalStateException("Cannot get exit code before executing command line: " + cl); + return new OsProcess(process, circular); } - return handler.getExitValue(); } - public void checkForError() { - if (handler.getException() != null) { - LOG.severe(handler.getException().toString()); - } + public static Builder builder() { + return new Builder(); } - public String getStdOut() { - return inputOut.toString(); - } + private final Process process; + private final CircularOutputStream outputStream; - public void setInput(String allInput) { - this.allInput = allInput; + public OsProcess(Process process, CircularOutputStream outputStream) { + this.process = process; + this.outputStream = outputStream; } - public void setWorkingDirectory(File workingDirectory) { - executor.setWorkingDirectory(workingDirectory); + /** + * The last N bytes of the combined stdout and stderr as String, the value of N is set while + * building the OsProcess. + * + * @return stdout and stderr as String in Charset.defaultCharset() encoding + */ + public String getOutput() { + return outputStream.toString(); } - @Override - public String toString() { - return cl.toString() + "[ " + env + "]"; + public boolean isAlive() { + return process.isAlive(); } - public void copyOutputTo(OutputStream out) { - drainTo = out; + public boolean waitFor(Duration duration) throws InterruptedException { + return process.waitFor(duration.toMillis(), TimeUnit.MILLISECONDS); } - class SeleniumWatchDog extends ExecuteWatchdog { - - private volatile Process process; - private volatile boolean starting = true; - - SeleniumWatchDog(long timeout) { - super(timeout); - } - - @Override - public synchronized void start(Process process) { - this.process = process; - starting = false; - super.start(process); - } + public int exitValue() { + return process.exitValue(); + } - public void reset() { - starting = true; - } + /** + * Initiate a normal shutdown of the process or kills it when the process is alive after 4 + * seconds. + */ + public void shutdown() { + if (process.supportsNormalTermination()) { + process.destroy(); - private boolean waitForProcessStarted(long duration, TimeUnit unit) { - long end = System.currentTimeMillis() + unit.toMillis(duration); - while (starting && System.currentTimeMillis() < end) { - try { - Thread.sleep(50); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new WebDriverException(e); - } - } - - return !starting; - } - - private void waitForTerminationAfterDestroy(int duration, TimeUnit unit) { - long end = System.currentTimeMillis() + unit.toMillis(duration); - while (isRunning() && System.currentTimeMillis() < end) { - try { - Thread.sleep(50); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new WebDriverException(e); + try { + if (process.waitFor(4, SECONDS)) { + return; } + } catch (InterruptedException ex) { + Thread.interrupted(); } } - private void destroyHarder() { - try { - Process awaitFor = this.process.destroyForcibly(); - awaitFor.waitFor(10, SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); - } - } + process.destroyForcibly(); } } diff --git a/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java b/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java index 3a3a0fe1b7a7d..c04e7dc00eedc 100644 --- a/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java +++ b/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java @@ -145,7 +145,11 @@ public Response execute(Command command) throws IOException { CompletableFuture processFinished = CompletableFuture.supplyAsync( () -> { - service.process.waitFor(service.getTimeout().toMillis()); + try { + service.process.waitFor(service.getTimeout()); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } return null; }, executorService); diff --git a/java/src/org/openqa/selenium/remote/service/DriverService.java b/java/src/org/openqa/selenium/remote/service/DriverService.java index f5313bf90ed8f..d0f49203888f6 100644 --- a/java/src/org/openqa/selenium/remote/service/DriverService.java +++ b/java/src/org/openqa/selenium/remote/service/DriverService.java @@ -49,7 +49,7 @@ import org.openqa.selenium.internal.Require; import org.openqa.selenium.net.PortProber; import org.openqa.selenium.net.UrlChecker; -import org.openqa.selenium.os.CommandLine; +import org.openqa.selenium.os.OsProcess; /** * Manages the life and death of a native executable driver server. It is expected that the driver @@ -93,7 +93,7 @@ public class DriverService implements Closeable { * A reference to the current child process. Will be {@code null} whenever this service is not * running. Protected by {@link #lock}. */ - protected CommandLine process = null; + protected OsProcess process = null; private OutputStream outputStream = System.err; @@ -173,7 +173,7 @@ public URL getUrl() { public boolean isRunning() { lock.lock(); try { - return process != null && process.isRunning(); + return process != null && process.isAlive(); } catch (IllegalThreadStateException e) { return true; } finally { @@ -201,13 +201,12 @@ public void start() throws IOException { this.executable = DriverFinder.getPath(this, getDefaultDriverOptions()).getDriverPath(); } LOG.fine(String.format("Starting driver at %s with %s", this.executable, this.args)); - process = new CommandLine(this.executable, args.toArray(new String[] {})); - process.setEnvironmentVariables(environment); - process.copyOutputTo(getOutputStream()); - process.executeAsync(); - if (!process.waitForProcessStarted(timeout.toMillis(), TimeUnit.MILLISECONDS)) { - throw new WebDriverException("Timed out waiting for driver process to start."); - } + + OsProcess.Builder builder = + OsProcess.builder().command(this.executable, args).copyOutputTo(getOutputStream()); + + environment.forEach(builder::environment); + process = builder.start(); CompletableFuture serverStarted = CompletableFuture.supplyAsync( @@ -221,11 +220,13 @@ public void start() throws IOException { CompletableFuture.supplyAsync( () -> { try { - process.waitFor(getTimeout().toMillis()); - } catch (org.openqa.selenium.TimeoutException ex) { - return StartOrDie.PROCESS_IS_ACTIVE; + return process.waitFor(getTimeout()) + ? StartOrDie.PROCESS_DIED + : StartOrDie.PROCESS_IS_ACTIVE; + } catch (InterruptedException ex) { + process.shutdown(); + return null; } - return StartOrDie.PROCESS_DIED; }, executorService); @@ -234,6 +235,11 @@ public void start() throws IOException { (StartOrDie) CompletableFuture.anyOf(serverStarted, processFinished) .get(getTimeout().toMillis() * 2, TimeUnit.MILLISECONDS); + + if (status == null) { + throw new InterruptedException(); + } + switch (status) { case SERVER_STARTED: processFinished.cancel(true); @@ -244,11 +250,16 @@ public void start() throws IOException { case PROCESS_IS_ACTIVE: throw new WebDriverException("Timed out waiting for driver server to bind the port."); } - } catch (ExecutionException | TimeoutException e) { + } catch (ExecutionException e) { + process.shutdown(); + throw new WebDriverException("Failed waiting for driver server to start.", e); + } catch (TimeoutException e) { + process.shutdown(); throw new WebDriverException("Timed out waiting for driver server to start.", e); } catch (InterruptedException e) { + process.shutdown(); Thread.currentThread().interrupt(); - throw new WebDriverException("Timed out waiting for driver server to start.", e); + throw new WebDriverException("Interrupted while waiting for driver server to start.", e); } } finally { lock.unlock(); @@ -296,7 +307,7 @@ public void stop() { } } - process.destroy(); + process.shutdown(); if (getOutputStream() instanceof FileOutputStream) { try { diff --git a/java/test/org/openqa/selenium/build/BazelBuild.java b/java/test/org/openqa/selenium/build/BazelBuild.java index b50ff01f885a8..977de842542bd 100644 --- a/java/test/org/openqa/selenium/build/BazelBuild.java +++ b/java/test/org/openqa/selenium/build/BazelBuild.java @@ -22,9 +22,10 @@ import java.io.File; import java.nio.file.Files; import java.nio.file.Path; +import java.time.Duration; import java.util.logging.Logger; import org.openqa.selenium.WebDriverException; -import org.openqa.selenium.os.CommandLine; +import org.openqa.selenium.os.OsProcess; public class BazelBuild { private static final Logger LOG = Logger.getLogger(BazelBuild.class.getName()); @@ -58,13 +59,23 @@ public void build(String target) { } LOG.info("\nBuilding " + target + " ..."); - CommandLine commandLine = new CommandLine("bazel", "build", target); - commandLine.setWorkingDirectory(projectRoot.toAbsolutePath().toString()); - commandLine.copyOutputTo(System.err); - commandLine.execute(); + OsProcess process = + OsProcess.builder() + .command("bazel", "build", target) + .directory(projectRoot.toAbsolutePath().toString()) + .copyOutputTo(System.err) + .start(); - if (!commandLine.isSuccessful()) { - throw new WebDriverException("Build failed! " + target + "\n" + commandLine.getStdOut()); + try { + if (process.waitFor(Duration.ofHours(1))) { + if (process.exitValue() != 0) + throw new WebDriverException("Build failed! " + target + "\n" + process.getOutput()); + } else { + throw new WebDriverException("Build timed out! " + target + "\n" + process.getOutput()); + } + } catch (InterruptedException ex) { + process.shutdown(); + throw new WebDriverException("Build interrupted! " + target + "\n" + process.getOutput()); } } } diff --git a/java/test/org/openqa/selenium/os/CommandLineTest.java b/java/test/org/openqa/selenium/os/CommandLineTest.java deleted file mode 100644 index 1579868bf3bce..0000000000000 --- a/java/test/org/openqa/selenium/os/CommandLineTest.java +++ /dev/null @@ -1,171 +0,0 @@ -// Licensed to the Software Freedom Conservancy (SFC) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The SFC licenses this file -// to you 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. - -package org.openqa.selenium.os; - -import static java.lang.System.getenv; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assumptions.assumeThat; -import static org.junit.jupiter.api.Assumptions.assumeTrue; -import static org.mockito.Mockito.atLeastOnce; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.verifyNoMoreInteractions; -import static org.openqa.selenium.Platform.WINDOWS; -import static org.openqa.selenium.os.CommandLine.getLibraryPathPropertyName; -import static org.openqa.selenium.testing.TestUtilities.isOnTravis; - -import java.io.ByteArrayOutputStream; -import java.io.File; -import java.lang.reflect.Field; -import java.util.HashMap; -import java.util.Map; -import org.junit.jupiter.api.Test; -import org.openqa.selenium.Platform; -import org.openqa.selenium.build.BazelBuild; - -class CommandLineTest { - - // ping can be found on every platform we support. - private static final String testExecutable = - findExecutable("java/test/org/openqa/selenium/os/echo"); - - private final CommandLine commandLine = new CommandLine(testExecutable); - private final OsProcess process = spyProcess(commandLine); - - @Test - void testSetEnvironmentVariableDelegatesToProcess() { - String key = "foo"; - String value = "bar"; - commandLine.setEnvironmentVariable(key, value); - verify(process).setEnvironmentVariable(key, value); - verifyNoMoreInteractions(process); - } - - @Test - void testSetEnvironmentVariablesDelegatesToProcess() { - Map env = new HashMap<>(); - env.put("k1", "v1"); - env.put("k2", "v2"); - commandLine.setEnvironmentVariables(env); - verify(process).setEnvironmentVariable("k1", "v1"); - verify(process).setEnvironmentVariable("k2", "v2"); - verifyNoMoreInteractions(process); - } - - @Test - void testSetDynamicLibraryPathWithNullValueIgnores() { - commandLine.setDynamicLibraryPath(null); - verifyNoInteractions(process); - } - - @Test - void testSetDynamicLibraryPathWithNonNullValueSets() { - String value = "Bar"; - commandLine.setDynamicLibraryPath(value); - verify(process).setEnvironmentVariable(getLibraryPathPropertyName(), value); - verifyNoMoreInteractions(process); - } - - @Test - void executeWaitsForProcessFinish() throws InterruptedException { - commandLine.execute(); - verify(process).executeAsync(); - verify(process).waitFor(); - verifyNoMoreInteractions(process); - } - - @Test - void testDestroy() { - commandLine.executeAsync(); - verify(process).executeAsync(); - assertThat(commandLine.isRunning()).isTrue(); - verify(process).isRunning(); - commandLine.destroy(); - verify(process).destroy(); - assertThat(commandLine.isRunning()).isFalse(); - verify(process, atLeastOnce()).isRunning(); - } - - @Test - void canHandleOutput() { - CommandLine commandLine = new CommandLine(testExecutable, "ping"); - commandLine.execute(); - assertThat(commandLine.getStdOut()).isNotEmpty().contains("ping"); - } - - @Test - void canCopyOutput() { - CommandLine commandLine = new CommandLine(testExecutable, "I", "love", "cheese"); - - ByteArrayOutputStream buffer = new ByteArrayOutputStream(); - commandLine.copyOutputTo(buffer); - commandLine.execute(); - assertThat(buffer.toByteArray()).isNotEmpty(); - assertThat(commandLine.getStdOut()).isEqualTo(buffer.toString()); - } - - @Test - void canDetectSuccess() { - assumeThat(isOnTravis()).as("Operation not permitted on travis").isFalse(); - CommandLine commandLine = - new CommandLine( - testExecutable, (Platform.getCurrent().is(WINDOWS) ? "-n" : "-c"), "3", "localhost"); - commandLine.execute(); - assertThat(commandLine.getExitCode()).isZero(); - assertThat(commandLine.isSuccessful()).isTrue(); - } - - @Test - void canDetectFailure() { - commandLine.execute(); - assertThat(commandLine.getExitCode()).isNotEqualTo(0); - assertThat(commandLine.isSuccessful()).isFalse(); - } - - @Test - void canUpdateLibraryPath() { - assumeTrue(Platform.getCurrent().is(WINDOWS)); - commandLine.updateDynamicLibraryPath("C:\\My\\Tools"); - verify(process) - .setEnvironmentVariable( - getLibraryPathPropertyName(), String.format("%s;%s", getenv("PATH"), "C:\\My\\Tools")); - } - - private OsProcess spyProcess(CommandLine commandLine) { - try { - Field processField = CommandLine.class.getDeclaredField("process"); - processField.setAccessible(true); - OsProcess process = (OsProcess) processField.get(commandLine); - OsProcess spyProcess = spy(process); - processField.set(commandLine, spyProcess); - return spyProcess; - } catch (NoSuchFieldException | IllegalAccessException e) { - throw new RuntimeException(e); - } - } - - private static String findExecutable(String relativePath) { - if (Platform.getCurrent().is(Platform.WINDOWS)) { - File workingDir = BazelBuild.findBinRoot(new File(".").getAbsoluteFile()); - return new File(workingDir, relativePath).getAbsolutePath(); - } else { - return relativePath; - } - } -} diff --git a/java/test/org/openqa/selenium/os/OsProcessTest.java b/java/test/org/openqa/selenium/os/OsProcessTest.java index 4d3484c5277b7..5d71034f10928 100644 --- a/java/test/org/openqa/selenium/os/OsProcessTest.java +++ b/java/test/org/openqa/selenium/os/OsProcessTest.java @@ -19,12 +19,10 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assumptions.assumeThat; -import static org.openqa.selenium.Platform.WINDOWS; -import static org.openqa.selenium.testing.TestUtilities.isOnTravis; import java.io.ByteArrayOutputStream; import java.io.File; +import java.time.Duration; import org.junit.jupiter.api.Test; import org.openqa.selenium.Platform; import org.openqa.selenium.build.BazelBuild; @@ -34,77 +32,85 @@ class OsProcessTest { private static final String testExecutable = findExecutable("java/test/org/openqa/selenium/os/echo"); - private OsProcess process = new OsProcess(testExecutable); - @Test void testSetEnvironmentVariableWithNullKeyThrows() { + OsProcess.Builder builder = OsProcess.builder().command(testExecutable); + var environment = builder.environment(); + String key = null; String value = "Bar"; assertThatExceptionOfType(IllegalArgumentException.class) - .isThrownBy(() -> process.setEnvironmentVariable(key, value)); - assertThat(process.getEnvironment()).doesNotContainValue(value); + .isThrownBy(() -> builder.environment(key, value)); + assertThat(environment).doesNotContainValue(value); } @Test void testSetEnvironmentVariableWithNullValueThrows() { + OsProcess.Builder builder = OsProcess.builder().command(testExecutable); + var environment = builder.environment(); + String key = "Foo"; String value = null; assertThatExceptionOfType(IllegalArgumentException.class) - .isThrownBy(() -> process.setEnvironmentVariable(key, value)); - assertThat(process.getEnvironment()).doesNotContainKey(key); + .isThrownBy(() -> builder.environment(key, value)); + assertThat(environment).doesNotContainKey(key); } @Test void testSetEnvironmentVariableWithNonNullValueSets() { + OsProcess.Builder builder = OsProcess.builder().command(testExecutable); + var environment = builder.environment(); + String key = "Foo"; String value = "Bar"; - process.setEnvironmentVariable(key, value); - assertThat(process.getEnvironment()).containsEntry(key, value); + builder.environment(key, value); + assertThat(environment).containsEntry(key, value); } @Test void testDestroy() { - process.executeAsync(); - assertThat(process.isRunning()).isTrue(); - process.destroy(); - assertThat(process.isRunning()).isFalse(); + OsProcess process = OsProcess.builder().command(testExecutable).start(); + assertThat(process.isAlive()).isTrue(); + process.shutdown(); + assertThat(process.isAlive()).isFalse(); } @Test void canHandleOutput() throws InterruptedException { - process = new OsProcess(testExecutable, "ping"); - process.executeAsync(); - process.waitFor(); - assertThat(process.getStdOut()).isNotEmpty().contains("ping"); + OsProcess process = OsProcess.builder().command(testExecutable, "ping").start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.getOutput()).isNotEmpty().contains("ping"); } @Test void canCopyOutput() throws InterruptedException { - process = new OsProcess(testExecutable, "Who", "else", "likes", "cheese?"); ByteArrayOutputStream buffer = new ByteArrayOutputStream(); - process.copyOutputTo(buffer); - process.executeAsync(); - process.waitFor(); + OsProcess process = + OsProcess.builder() + .command(testExecutable, "Who", "else", "likes", "cheese?") + .copyOutputTo(buffer) + .start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); assertThat(buffer.toByteArray()).isNotEmpty(); - assertThat(process.getStdOut()).isEqualTo(buffer.toString()); + assertThat(process.getOutput()).isEqualTo(buffer.toString()); } @Test void canDetectSuccess() throws InterruptedException { - assumeThat(isOnTravis()).as("Operation not permitted on travis").isFalse(); - OsProcess process = - new OsProcess( - testExecutable, (Platform.getCurrent().is(WINDOWS) ? "-n" : "-c"), "3", "localhost"); - process.executeAsync(); - process.waitFor(); - assertThat(process.getExitCode()).isZero(); + OsProcess process = OsProcess.builder().command(testExecutable, "foo").start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.exitValue()).isZero(); } @Test void canDetectFailure() throws InterruptedException { - process.executeAsync(); - process.waitFor(); - assertThat(process.getExitCode()).isNotEqualTo(0); + OsProcess process = OsProcess.builder().command(testExecutable).start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.exitValue()).isNotEqualTo(0); } private static String findExecutable(String relativePath) { diff --git a/java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java b/java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java index f7e9638892196..d389db0102037 100644 --- a/java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java +++ b/java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Objects; import java.util.logging.Logger; +import java.util.stream.Collectors; import java.util.stream.Stream; import org.openqa.selenium.Platform; import org.openqa.selenium.build.BazelBuild; @@ -38,7 +39,7 @@ import org.openqa.selenium.net.NetworkUtils; import org.openqa.selenium.net.PortProber; import org.openqa.selenium.net.UrlChecker; -import org.openqa.selenium.os.CommandLine; +import org.openqa.selenium.os.OsProcess; import org.openqa.selenium.remote.service.DriverService; class OutOfProcessSeleniumServer { @@ -46,7 +47,7 @@ class OutOfProcessSeleniumServer { private static final Logger LOG = Logger.getLogger(OutOfProcessSeleniumServer.class.getName()); private String baseUrl; - private CommandLine command; + private OsProcess process; @SuppressWarnings("unused") private boolean captureLogs = false; @@ -62,7 +63,7 @@ public void enableLogCapture() { */ public OutOfProcessSeleniumServer start(String mode, String... extraFlags) { LOG.info("Got a request to start a new selenium server"); - if (command != null) { + if (process != null) { LOG.info("Server already started"); throw new RuntimeException("Server already started"); } @@ -105,23 +106,25 @@ public OutOfProcessSeleniumServer start(String mode, String... extraFlags) { startupArgs.add("true"); } - command = - new CommandLine( - serverBinary, - Stream.concat( - javaFlags, - Stream.concat( - // If the driver is provided, we _don't_ want to use Selenium Manager - startupArgs.stream(), Stream.of(extraFlags))) - .toArray(String[]::new)); + OsProcess.Builder builder = + OsProcess.builder() + .command( + serverBinary, + Stream.concat( + javaFlags, + Stream.concat( + // If the driver is provided, we _don't_ want to use Selenium Manager + startupArgs.stream(), Stream.of(extraFlags))) + .collect(Collectors.toList())) + .copyOutputTo(System.err); + if (Platform.getCurrent().is(Platform.WINDOWS)) { File workingDir = findBinRoot(new File(".").getAbsoluteFile()); - command.setWorkingDirectory(workingDir.getAbsolutePath()); + builder.directory(workingDir.getAbsolutePath()); } - command.copyOutputTo(System.err); - LOG.info("Starting selenium server: " + command.toString()); - command.executeAsync(); + LOG.info("Starting selenium server: " + builder.command()); + process = builder.start(); try { URL url = new URL(baseUrl + "/status"); @@ -130,9 +133,9 @@ public OutOfProcessSeleniumServer start(String mode, String... extraFlags) { LOG.info("Server is ready"); } catch (UrlChecker.TimeoutException e) { LOG.severe("Server failed to start: " + e.getMessage()); - command.destroy(); - LOG.severe(command.getStdOut()); - command = null; + process.shutdown(); + LOG.severe(process.getOutput()); + process = null; throw new RuntimeException(e); } catch (MalformedURLException e) { throw new RuntimeException(e); @@ -152,13 +155,13 @@ private File findBinRoot(File dir) { } public void stop() { - if (command == null) { + if (process == null) { return; } LOG.info("Stopping selenium server"); - command.destroy(); + process.shutdown(); LOG.info("Selenium server stopped"); - command = null; + process = null; } private String buildServerAndClasspath() {