Permalink
Browse files

Kill the process before attempting to drain the streams

The output stream for a process is only closed once the
process has quit. Gosh knows how we managed to get this
to work before, but we now do this.
  • Loading branch information...
shs96c committed Jun 28, 2017
1 parent 55de2d3 commit 8981d39635493733ef8938bad9276b96b863de4a
Showing with 18 additions and 11 deletions.
  1. +18 −11 java/client/src/org/openqa/selenium/os/UnixProcess.java
@@ -42,6 +42,7 @@
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
class UnixProcess implements OsProcess {
@@ -115,6 +116,19 @@ public int destroy() {
SeleniumWatchDog watchdog = executeWatchdog;
watchdog.waitForProcessStarted();
// 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 (!thisIsWindows()) {
watchdog.destroyProcess();
watchdog.waitForTerminationAfterDestroy(2, SECONDS);
}
if (isRunning()) {
watchdog.destroyHarder();
watchdog.waitForTerminationAfterDestroy(1, SECONDS);
}
// 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.
@@ -123,20 +137,13 @@ public int destroy() {
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);
}
}
if (!thisIsWindows()) {
watchdog.destroyProcess();
watchdog.waitForTerminationAfterDestroy(2, SECONDS);
if (!isRunning()) {
return getExitCode();
}
log.info("Command failed to close cleanly. Destroying forcefully (v2). " + this);
}
watchdog.destroyHarder();
watchdog.waitForTerminationAfterDestroy(1, SECONDS);
if (!isRunning()) {
return getExitCode();
}

1 comment on commit 8981d39

@juangj

This comment has been minimized.

Show comment
Hide comment
@juangj

juangj Jul 13, 2017

Contributor

Hm... so, the reason I changed this in f58cb8f to stop reading first, and then kill the process was because ThreadSanitizer was complaining about a data race when simultaneously trying to read a stream and close it.

What was the reason for doing this the other way? Were we missing output from right before the process died?

Contributor

juangj commented on 8981d39 Jul 13, 2017

Hm... so, the reason I changed this in f58cb8f to stop reading first, and then kill the process was because ThreadSanitizer was complaining about a data race when simultaneously trying to read a stream and close it.

What was the reason for doing this the other way? Were we missing output from right before the process died?

Please sign in to comment.