Implement timeouts on top of Java Process PiperOrigin-RevId: 164827022
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Command.java b/src/main/java/com/google/devtools/build/lib/shell/Command.java index f33f53d..8413441 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/Command.java +++ b/src/main/java/com/google/devtools/build/lib/shell/Command.java
@@ -122,17 +122,6 @@ public static final boolean KILL_SUBPROCESS_ON_INTERRUPT = true; public static final boolean CONTINUE_SUBPROCESS_ON_INTERRUPT = false; - private static final KillableObserver NO_OBSERVER = new KillableObserver() { - @Override - public void startObserving(final Killable killable) { - // do nothing - } - @Override - public void stopObserving(final Killable killable) { - // do nothing - } - }; - private final SubprocessBuilder subprocessBuilder; /** @@ -380,24 +369,10 @@ // enforced. processInput(stdinInput, process); - final KillableObserver theObserver; - if ((subprocessBuilder.getTimeoutMillis() > 0) - && !SubprocessBuilder.factory.supportsTimeout()) { - theObserver = new TimeoutKillableObserver(subprocessBuilder.getTimeoutMillis()); - } else { - theObserver = NO_OBSERVER; - } - Killable processKillable = observeProcess(process, theObserver); - return new FutureCommandResult() { @Override public CommandResult get() throws AbnormalTerminationException { - return waitForProcessToComplete( - process, - theObserver, - processKillable, - outErrConsumers, - killSubprocessOnInterrupt); + return waitForProcessToComplete(process, outErrConsumers, killSubprocessOnInterrupt); } @Override @@ -450,23 +425,12 @@ } } - private static Killable observeProcess(Subprocess process, KillableObserver observer) { - Killable processKillable = new ProcessKillable(process); - observer.startObserving(processKillable); - return processKillable; - } - private CommandResult waitForProcessToComplete( - Subprocess process, - KillableObserver observer, - Killable processKillable, - Consumers.OutErrConsumers outErr, - boolean killSubprocessOnInterrupt) + Subprocess process, Consumers.OutErrConsumers outErr, boolean killSubprocessOnInterrupt) throws AbnormalTerminationException { log.finer("Waiting for process..."); TerminationStatus status = waitForProcess(process, killSubprocessOnInterrupt); - observer.stopObserving(processKillable); log.finer(status.toString()); @@ -495,9 +459,6 @@ noOutputResult, message, ioe); } } finally { - // #close() must be called after the #stopObserving() so that a badly-timed timeout does not - // try to destroy a process that is already closed, and after outErr is completed, - // so that it has a chance to read the entire output is captured. process.close(); }
diff --git a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java index e266cc9..28464ba 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java +++ b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
@@ -20,6 +20,8 @@ import java.io.InputStream; import java.io.OutputStream; import java.lang.ProcessBuilder.Redirect; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; /** * A subprocess factory that uses {@link java.lang.ProcessBuilder}. @@ -31,9 +33,12 @@ */ private static class JavaSubprocess implements Subprocess { private final Process process; + private final long deadlineMillis; + private final AtomicBoolean deadlineExceeded = new AtomicBoolean(); - private JavaSubprocess(Process process) { + private JavaSubprocess(Process process, long deadlineMillis) { this.process = process; + this.deadlineMillis = deadlineMillis; } @Override @@ -50,6 +55,13 @@ @Override public boolean finished() { try { + if (deadlineMillis > 0 + && System.currentTimeMillis() > deadlineMillis + && deadlineExceeded.compareAndSet(false, true)) { + // We use compareAndSet here to avoid calling destroy multiple times. Note that destroy + // returns immediately, and we don't want to wait in this method. + process.destroy(); + } // this seems to be the only non-blocking call for checking liveness process.exitValue(); return true; @@ -60,13 +72,28 @@ @Override public boolean timedout() { - // Not supported. - return false; + return deadlineExceeded.get(); } @Override public void waitFor() throws InterruptedException { - process.waitFor(); + if (deadlineMillis > 0) { + // Careful: I originally used Long.MAX_VALUE if there's no timeout. This is safe with + // Process, but not for the UNIXProcess subclass, which has an integer overflow for very + // large timeouts. As of this writing, it converts the passed in value to nanos (which + // saturates at Long.MAX_VALUE), then adds 999999 to round up (which overflows), converts + // back to millis, and then calls Object.wait with a negative timeout, which throws. + long waitTimeMillis = deadlineMillis - System.currentTimeMillis(); + boolean exitedInTime = process.waitFor(waitTimeMillis, TimeUnit.MILLISECONDS); + if (!exitedInTime && deadlineExceeded.compareAndSet(false, true)) { + process.destroy(); + // The destroy call returns immediately, so we still need to wait for the actual exit. The + // sole caller assumes that waitFor only exits when the process is gone (or throws). + process.waitFor(); + } + } else { + process.waitFor(); + } } @Override @@ -86,7 +113,8 @@ @Override public void close() { - // java.lang.Process doesn't give us a way to clean things up other than #destroy() + // java.lang.Process doesn't give us a way to clean things up other than #destroy(), which was + // already called by this point. } } @@ -97,11 +125,6 @@ } @Override - public boolean supportsTimeout() { - return false; - } - - @Override public Subprocess create(SubprocessBuilder params) throws IOException { ProcessBuilder builder = new ProcessBuilder(); builder.command(params.getArgv()); @@ -114,7 +137,11 @@ builder.redirectError(getRedirect(params.getStderr(), params.getStderrFile())); builder.directory(params.getWorkingDirectory()); - return new JavaSubprocess(builder.start()); + // Deadline is now + given timeout. + long deadlineMillis = params.getTimeoutMillis() > 0 + ? Math.addExact(System.currentTimeMillis(), params.getTimeoutMillis()) + : 0; + return new JavaSubprocess(builder.start(), deadlineMillis); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Killable.java b/src/main/java/com/google/devtools/build/lib/shell/Killable.java deleted file mode 100644 index 82eec84..0000000 --- a/src/main/java/com/google/devtools/build/lib/shell/Killable.java +++ /dev/null
@@ -1,29 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// 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. - -package com.google.devtools.build.lib.shell; - -/** - * Implementations encapsulate a running process that can be killed. In particular, here, it is used - * to wrap up a {@link Process} object and expose it to a {@link KillableObserver}. It is wrapped in - * this way so that the actual {@link Process} object can't be altered by a - * {@link KillableObserver}. - */ -interface Killable { - - /** - * Kill this killable instance. - */ - void kill(); -}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/KillableObserver.java b/src/main/java/com/google/devtools/build/lib/shell/KillableObserver.java deleted file mode 100644 index 6afa57c..0000000 --- a/src/main/java/com/google/devtools/build/lib/shell/KillableObserver.java +++ /dev/null
@@ -1,38 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// 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. - -package com.google.devtools.build.lib.shell; - -/** - * Implementations of this interface observe, and potentially kill, a {@link Killable} object. - */ -interface KillableObserver { - /** - * Begin observing the given {@link Killable}. This method must return promptly; until it returns, - * {@link Command#execute()} cannot complete. Implementations may wish to start a new - * {@link Thread} here to handle kill logic, and to interrupt or otherwise ask the thread to stop - * in the {@link #stopObserving(Killable)} method. See - * <a href="http://builder.com.com/5100-6370-5144546.html">Interrupting Java threads</a> for notes - * on how to implement this correctly. - * - * <p>Implementations may or may not be able to observe more than one {@link Killable} at a time; - * see javadoc for details. - * - * @param killable killable to observer - */ - void startObserving(Killable killable); - - /** Stop observing the given {@link Killable}, since it is no longer active. */ - void stopObserving(Killable killable); -}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/ProcessKillable.java b/src/main/java/com/google/devtools/build/lib/shell/ProcessKillable.java deleted file mode 100644 index 76118df..0000000 --- a/src/main/java/com/google/devtools/build/lib/shell/ProcessKillable.java +++ /dev/null
@@ -1,36 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// 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. - -package com.google.devtools.build.lib.shell; - -/** - * {@link Killable} implementation which simply wraps a - * {@link Process} instance. - */ -final class ProcessKillable implements Killable { - - private final Subprocess process; - - ProcessKillable(Subprocess process) { - this.process = process; - } - - /** - * Calls {@link Subprocess#destroy()}. - */ - @Override - public void kill() { - process.destroy(); - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java b/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java index 13ad593..3b85aaf 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java +++ b/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java
@@ -29,12 +29,6 @@ */ interface Factory { /** - * Whether the factory supports timeouts natively; if it returns false, Command implements - * timeouts outside of the factory. - */ - boolean supportsTimeout(); - - /** * Create a subprocess according to the specified parameters. */ Subprocess create(SubprocessBuilder params) throws IOException;
diff --git a/src/main/java/com/google/devtools/build/lib/shell/TimeoutKillableObserver.java b/src/main/java/com/google/devtools/build/lib/shell/TimeoutKillableObserver.java deleted file mode 100644 index 44a3299..0000000 --- a/src/main/java/com/google/devtools/build/lib/shell/TimeoutKillableObserver.java +++ /dev/null
@@ -1,100 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// 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. - -package com.google.devtools.build.lib.shell; - -import java.util.logging.Level; -import java.util.logging.Logger; - -/** - * {@link KillableObserver} implementation which will kill its observed {@link Killable} if it is - * still being observed after a given amount of time has elapsed. - * - * <p>Note that this class can only observe one {@link Killable} at a time; multiple instances - * should be used for concurrent calls to {@link Command#execute}. - */ -final class TimeoutKillableObserver implements KillableObserver { - - private static final Logger log = - Logger.getLogger(TimeoutKillableObserver.class.getCanonicalName()); - - private final long timeoutMS; - private Killable killable; - private SleeperThread sleeperThread; - private boolean timedOut; - - // TODO(bazel-team): I'd like to use ThreadPool2, but it doesn't currently - // provide a way to interrupt a thread - - public TimeoutKillableObserver(final long timeoutMS) { - this.timeoutMS = timeoutMS; - } - - /** - * Starts a new {@link Thread} to wait for the timeout period. This is - * interrupted by the {@link #stopObserving(Killable)} method. - * - * @param killable killable to kill when the timeout period expires - */ - @Override - public synchronized void startObserving(final Killable killable) { - this.timedOut = false; - this.killable = killable; - this.sleeperThread = new SleeperThread(); - this.sleeperThread.start(); - } - - @Override - public synchronized void stopObserving(final Killable killable) { - if (!this.killable.equals(killable)) { - throw new IllegalStateException("start/stopObservering called with " + - "different Killables"); - } - if (sleeperThread.isAlive()) { - sleeperThread.interrupt(); - } - this.killable = null; - sleeperThread = null; - } - - private final class SleeperThread extends Thread { - @Override public void run() { - try { - if (log.isLoggable(Level.FINE)) { - log.fine("Waiting for " + timeoutMS + "ms to kill process"); - } - Thread.sleep(timeoutMS); - // timeout expired; kill it - synchronized (TimeoutKillableObserver.this) { - if (killable != null) { - log.fine("Killing process"); - killable.kill(); - timedOut = true; - } - } - } catch (InterruptedException ie) { - // continue -- process finished before timeout - log.fine("Wait interrupted since process finished; continuing..."); - } - } - } - - /** - * Returns true if the observed process was killed by this observer. - */ - public synchronized boolean hasTimedOut() { - // synchronized needed for memory model visibility. - return timedOut; - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java index d033bac..e398a3b 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
@@ -37,11 +37,6 @@ } @Override - public boolean supportsTimeout() { - return true; - } - - @Override public Subprocess create(SubprocessBuilder builder) throws IOException { List<String> argv = builder.getArgv();
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index 74a4193..e822904 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -135,11 +135,6 @@ private static final class SubprocessInterceptor implements Subprocess.Factory { @Override - public boolean supportsTimeout() { - return true; - } - - @Override public Subprocess create(SubprocessBuilder params) throws IOException { throw new UnsupportedOperationException(); }
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java index a4aab86..52dd8b9 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java
@@ -44,9 +44,6 @@ @RunWith(JUnit4.class) public class CommandTest { - private static final long LONG_TIME = 10000; - private static final long SHORT_TIME = 250; - // Platform-independent tests ---------------------------------------------- @Before @@ -285,7 +282,7 @@ new Command(args).execute(); fail("Should have exited with status " + exit); } catch (BadExitStatusException e) { - assertThat(e).hasMessage("Process exited with status " + exit); + assertThat(e).hasMessageThat().isEqualTo("Process exited with status " + exit); checkCommandElements(e, "/bin/sh", "-c", "exit " + exit); TerminationStatus status = e.getResult().getTerminationStatus(); assertThat(status.success()).isFalse(); @@ -303,7 +300,7 @@ new Command(args).execute(); fail("Should have exited with status " + expected); } catch (BadExitStatusException e) { - assertThat(e).hasMessage("Process exited with status " + expected); + assertThat(e).hasMessageThat().isEqualTo("Process exited with status " + expected); checkCommandElements(e, "/bin/bash", "-c", "exit " + exit); TerminationStatus status = e.getResult().getTerminationStatus(); assertThat(status.success()).isFalse(); @@ -328,7 +325,7 @@ new Command(args).execute(); fail("Expected signal " + signal); } catch (AbnormalTerminationException e) { - assertThat(e).hasMessage("Process terminated by signal " + signal); + assertThat(e).hasMessageThat().isEqualTo("Process terminated by signal " + signal); checkCommandElements(e, killmyself, "" + signal); TerminationStatus status = e.getResult().getTerminationStatus(); assertThat(status.success()).isFalse(); @@ -440,86 +437,6 @@ } } - /** - * Helper to test KillableObserver classes. - */ - private static class KillableTester implements Killable { - private boolean isKilled = false; - private boolean timedOut = false; - @Override - public synchronized void kill() { - isKilled = true; - notifyAll(); - } - public synchronized boolean getIsKilled() { - return isKilled; - } - /** - * Wait for a specified time or until the {@link #kill()} is called. - */ - public synchronized void sleepUntilKilled(final long timeoutMS) { - long nowTime = System.currentTimeMillis(); - long endTime = nowTime + timeoutMS; - while (!isKilled && !timedOut) { - long waitTime = endTime - nowTime; - if (waitTime <= 0) { - // Process has timed out, needs killing. - timedOut = true; - break; - } - try { - wait(waitTime); // Suffers "spurious wakeup", hence the while() loop. - nowTime = System.currentTimeMillis(); - } catch (InterruptedException exception) { - break; - } - } - } - } - - @Test - public void testTimeOutKillableObserverNoKill() throws Exception { - KillableTester killable = new KillableTester(); - TimeoutKillableObserver observer = new TimeoutKillableObserver(LONG_TIME); - observer.startObserving(killable); - observer.stopObserving(killable); - assertThat(observer.hasTimedOut()).isFalse(); - assertThat(killable.getIsKilled()).isFalse(); - } - - @Test - public void testTimeOutKillableObserverNoKillWithDelay() throws Exception { - KillableTester killable = new KillableTester(); - TimeoutKillableObserver observer = new TimeoutKillableObserver(LONG_TIME); - observer.startObserving(killable); - killable.sleepUntilKilled(SHORT_TIME); - observer.stopObserving(killable); - assertThat(observer.hasTimedOut()).isFalse(); - assertThat(killable.getIsKilled()).isFalse(); - } - - @Test - public void testTimeOutKillableObserverWithKill() throws Exception { - KillableTester killable = new KillableTester(); - TimeoutKillableObserver observer = new TimeoutKillableObserver(SHORT_TIME); - observer.startObserving(killable); - killable.sleepUntilKilled(LONG_TIME); - observer.stopObserving(killable); - assertThat(observer.hasTimedOut()).isTrue(); - assertThat(killable.getIsKilled()).isTrue(); - } - - @Test - public void testTimeOutKillableObserverWithKillZeroMillis() throws Exception { - KillableTester killable = new KillableTester(); - TimeoutKillableObserver observer = new TimeoutKillableObserver(0); - observer.startObserving(killable); - killable.sleepUntilKilled(LONG_TIME); - observer.stopObserving(killable); - assertThat(observer.hasTimedOut()).isTrue(); - assertThat(killable.getIsKilled()).isTrue(); - } - private static void checkCommandElements(CommandException e, String... expected) { assertArrayEquals(expected, e.getCommand().getCommandLineElements());
diff --git a/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java b/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java deleted file mode 100644 index 3a2e4a6..0000000 --- a/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java +++ /dev/null
@@ -1,47 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// 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. - -package com.google.devtools.build.lib.shell; - -/** - * A simple implementation of {@link KillableObserver} which can be told explicitly to kill its - * {@link Killable} by calling {@link #kill()}. This is the sort of functionality that callers might - * expect to find available on the {@link Command} class.</p> - * - * <p>Note that this class can only observe one {@link Killable} at a time; multiple instances - * should be used for concurrent calls to {@link Command#execute}. - */ -final class SimpleKillableObserver implements KillableObserver { - private Killable killable; - - @Override - public synchronized void startObserving(final Killable killable) { - this.killable = killable; - } - - @Override - public synchronized void stopObserving(final Killable killable) { - if (!this.killable.equals(killable)) { - throw new IllegalStateException("start/stopObservering called with " + - "different Killables"); - } - this.killable = null; - } - - public synchronized void kill() { - if (killable != null) { - killable.kill(); - } - } -}