diff --git a/java/src/org/openqa/selenium/manager/SeleniumManager.java b/java/src/org/openqa/selenium/manager/SeleniumManager.java index f6d8b02c35252..9547b4204e220 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.ExternalProcess; /** * 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"); + ExternalProcess process = + ExternalProcess.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/CommandLine.java b/java/src/org/openqa/selenium/os/CommandLine.java index 56520b2c93bc3..e78611daac4d3 100644 --- a/java/src/org/openqa/selenium/os/CommandLine.java +++ b/java/src/org/openqa/selenium/os/CommandLine.java @@ -27,6 +27,7 @@ import org.openqa.selenium.Platform; import org.openqa.selenium.WebDriverException; +@Deprecated public class CommandLine { private final OsProcess process; diff --git a/java/src/org/openqa/selenium/os/ExternalProcess.java b/java/src/org/openqa/selenium/os/ExternalProcess.java new file mode 100644 index 0000000000000..632803e7afc1d --- /dev/null +++ b/java/src/org/openqa/selenium/os/ExternalProcess.java @@ -0,0 +1,286 @@ +// 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.util.concurrent.TimeUnit.SECONDS; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +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.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; +import org.openqa.selenium.internal.Require; +import org.openqa.selenium.io.CircularOutputStream; +import org.openqa.selenium.io.MultiOutputStream; + +public class ExternalProcess { + private static final Logger LOG = Logger.getLogger(ExternalProcess.class.getName()); + + public static class Builder { + + private ProcessBuilder builder; + private OutputStream copyOutputTo; + private int bufferSize = 32768; + + Builder() { + this.builder = new ProcessBuilder(); + } + + /** + * 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; + } + + /** + * 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; + } + + /** + * 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; + } + + /** + * 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()); + } + + /** + * 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(); + + builder.environment().put(name, value); + + return this; + } + + /** + * 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(); + } + + /** + * Get the working directory of the process to start, maybe null. + * + * @return the working directory + */ + public File directory() { + return builder.directory(); + } + + /** + * 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)); + } + + /** + * 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); + + return this; + } + + /** + * 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; + } + + /** + * 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; + + return this; + } + + public ExternalProcess start() throws UncheckedIOException { + // redirect the stderr to stdout + builder.redirectErrorStream(true); + + Process process; + try { + process = builder.start(); + } catch (IOException ex) { + throw new UncheckedIOException(ex); + } + + 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; + } + + return new ExternalProcess(process, circular); + } + } + + public static Builder builder() { + return new Builder(); + } + + private final Process process; + private final CircularOutputStream outputStream; + + public ExternalProcess(Process process, CircularOutputStream outputStream) { + this.process = process; + this.outputStream = outputStream; + } + + /** + * 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(); + } + + public boolean isAlive() { + return process.isAlive(); + } + + public boolean waitFor(Duration duration) throws InterruptedException { + return process.waitFor(duration.toMillis(), TimeUnit.MILLISECONDS); + } + + public int exitValue() { + return process.exitValue(); + } + + /** + * 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(); + + try { + if (process.waitFor(4, SECONDS)) { + return; + } + } catch (InterruptedException ex) { + Thread.interrupted(); + } + } + + process.destroyForcibly(); + } +} diff --git a/java/src/org/openqa/selenium/os/OsProcess.java b/java/src/org/openqa/selenium/os/OsProcess.java index a2786a8f1bd31..54847a94371a0 100644 --- a/java/src/org/openqa/selenium/os/OsProcess.java +++ b/java/src/org/openqa/selenium/os/OsProcess.java @@ -44,6 +44,7 @@ import org.openqa.selenium.io.CircularOutputStream; import org.openqa.selenium.io.MultiOutputStream; +@Deprecated class OsProcess { private static final Logger LOG = Logger.getLogger(OsProcess.class.getName()); 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..270367c7bd477 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.ExternalProcess; /** * 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 ExternalProcess 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."); - } + + ExternalProcess.Builder builder = + ExternalProcess.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..850f480e41edf 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.ExternalProcess; 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(); + ExternalProcess process = + ExternalProcess.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/ExternalProcessTest.java b/java/test/org/openqa/selenium/os/ExternalProcessTest.java new file mode 100644 index 0000000000000..201e8385dd601 --- /dev/null +++ b/java/test/org/openqa/selenium/os/ExternalProcessTest.java @@ -0,0 +1,124 @@ +// 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.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + +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; + +class ExternalProcessTest { + + private static final String testExecutable = + findExecutable("java/test/org/openqa/selenium/os/echo"); + + @Test + void testSetEnvironmentVariableWithNullKeyThrows() { + ExternalProcess.Builder builder = ExternalProcess.builder().command(testExecutable); + var environment = builder.environment(); + + String key = null; + String value = "Bar"; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> builder.environment(key, value)); + assertThat(environment).doesNotContainValue(value); + } + + @Test + void testSetEnvironmentVariableWithNullValueThrows() { + ExternalProcess.Builder builder = ExternalProcess.builder().command(testExecutable); + var environment = builder.environment(); + + String key = "Foo"; + String value = null; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> builder.environment(key, value)); + assertThat(environment).doesNotContainKey(key); + } + + @Test + void testSetEnvironmentVariableWithNonNullValueSets() { + ExternalProcess.Builder builder = ExternalProcess.builder().command(testExecutable); + var environment = builder.environment(); + + String key = "Foo"; + String value = "Bar"; + builder.environment(key, value); + assertThat(environment).containsEntry(key, value); + } + + @Test + void testDestroy() { + ExternalProcess process = ExternalProcess.builder().command(testExecutable).start(); + assertThat(process.isAlive()).isTrue(); + process.shutdown(); + assertThat(process.isAlive()).isFalse(); + } + + @Test + void canHandleOutput() throws InterruptedException { + ExternalProcess process = ExternalProcess.builder().command(testExecutable, "ping").start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.getOutput()).isNotEmpty().contains("ping"); + } + + @Test + void canCopyOutput() throws InterruptedException { + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + ExternalProcess process = + ExternalProcess.builder() + .command(testExecutable, "Who", "else", "likes", "cheese?") + .copyOutputTo(buffer) + .start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(buffer.toByteArray()).isNotEmpty(); + assertThat(process.getOutput()).isEqualTo(buffer.toString()); + } + + @Test + void canDetectSuccess() throws InterruptedException { + ExternalProcess process = ExternalProcess.builder().command(testExecutable, "foo").start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.exitValue()).isZero(); + } + + @Test + void canDetectFailure() throws InterruptedException { + ExternalProcess process = ExternalProcess.builder().command(testExecutable).start(); + + assertThat(process.waitFor(Duration.ofSeconds(20))).isTrue(); + assertThat(process.exitValue()).isNotEqualTo(0); + } + + 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/testing/drivers/OutOfProcessSeleniumServer.java b/java/test/org/openqa/selenium/testing/drivers/OutOfProcessSeleniumServer.java index f7e9638892196..8d7eea847ea62 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.ExternalProcess; 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 ExternalProcess 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)); + ExternalProcess.Builder builder = + ExternalProcess.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() {