Implement timeouts on Windows. Makes #1664 much less acute. -- MOS_MIGRATED_REVID=130750731
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 79b850a..9f35d69 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
@@ -163,6 +163,16 @@ } /** + * Just like {@link Command(String, Map<String, String>, File, long), but without a timeout. + */ + public Command( + String[] commandLineElements, + Map<String, String> environmentVariables, + File workingDirectory) { + this(commandLineElements, environmentVariables, workingDirectory, -1); + } + + /** * Creates a new {@link Command} for the given command line elements. The * command line is executed without a shell. * @@ -180,15 +190,17 @@ * * @param commandLineElements elements of raw command line to execute * @param environmentVariables environment variables to replace JVM's - * environment variables; may be null + * environment variables; may be null * @param workingDirectory working directory for execution; if null, current - * working directory is used + * working directory is used + * @param timeoutMillis timeout in milliseconds. Only supported on Windows. * @throws IllegalArgumentException if commandLine is null or empty */ public Command( String[] commandLineElements, - final Map<String, String> environmentVariables, - final File workingDirectory) { + Map<String, String> environmentVariables, + File workingDirectory, + long timeoutMillis) { if (commandLineElements == null || commandLineElements.length == 0) { throw new IllegalArgumentException("command line is null or empty"); } @@ -203,6 +215,7 @@ subprocessBuilder.setArgv(ImmutableList.copyOf(commandLineElements)); subprocessBuilder.setEnv(environmentVariables); subprocessBuilder.setWorkingDirectory(workingDirectory); + subprocessBuilder.setTimeoutMillis(timeoutMillis); } /** @@ -676,7 +689,7 @@ final Consumers.OutErrConsumers outErrConsumers, final boolean killSubprocessOnInterrupt, final boolean closeOutputStreams) - throws CommandException { + throws CommandException { logCommand(); @@ -872,7 +885,7 @@ while (true) { try { process.waitFor(); - return new TerminationStatus(process.exitValue()); + return new TerminationStatus(process.exitValue(), process.timedout()); } catch (InterruptedException ie) { wasInterrupted = true; if (killSubprocessOnInterrupt) {
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 7c85cec..5218490 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
@@ -60,6 +60,12 @@ } @Override + public boolean timedout() { + // Not supported. + return false; + } + + @Override public void waitFor() throws InterruptedException { process.waitFor(); } @@ -93,6 +99,9 @@ @Override public Subprocess create(SubprocessBuilder params) throws IOException { + if (params.getTimeoutMillis() >= 0) { + throw new UnsupportedOperationException("Timeouts are not supported"); + } ProcessBuilder builder = new ProcessBuilder(); builder.command(params.getArgv()); if (params.getEnv() != null) {
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 e5710e7..769b6b5 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
@@ -53,6 +53,11 @@ boolean finished(); /** + * Returns if the process timed out. + */ + boolean timedout(); + + /** * Waits for the process to finish. */ void waitFor() throws InterruptedException;
diff --git a/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java b/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java index b89cf96..0b70e05 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java
@@ -45,6 +45,7 @@ private StreamAction stderrAction; private File stderrFile; private File workingDirectory; + private long timeoutMillis = -1; private static Subprocess.Factory factory = JavaSubprocessFactory.INSTANCE; @@ -115,6 +116,15 @@ return this; } + public SubprocessBuilder setTimeoutMillis(long timeoutMillis) { + this.timeoutMillis = timeoutMillis; + return this; + } + + public long getTimeoutMillis() { + return timeoutMillis; + } + public StreamAction getStderr() { return stderrAction; }
diff --git a/src/main/java/com/google/devtools/build/lib/shell/TerminationStatus.java b/src/main/java/com/google/devtools/build/lib/shell/TerminationStatus.java index c8ee015..2f75b5e 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/TerminationStatus.java +++ b/src/main/java/com/google/devtools/build/lib/shell/TerminationStatus.java
@@ -29,6 +29,7 @@ public final class TerminationStatus { private final int waitResult; + private final boolean timedout; /** * Values taken from the glibc strsignal(3) function. @@ -79,8 +80,9 @@ * * @param waitResult the value returned by {@link java.lang.Process#waitFor}. */ - public TerminationStatus(int waitResult) { + public TerminationStatus(int waitResult, boolean timedout) { this.waitResult = waitResult; + this.timedout = timedout; } /** @@ -110,7 +112,14 @@ * Returns true iff the process exited normally. */ public boolean exited() { - return waitResult < SIGNAL_1 || waitResult > SIGNAL_63; + return !timedout && (waitResult < SIGNAL_1 || waitResult > SIGNAL_63); + } + + /** + * Returns true if the process timed out. + */ + public boolean timedout() { + return timedout; } /** @@ -128,7 +137,7 @@ * if exited() returns true. */ public int getTerminatingSignal() { - if (exited()) { + if (exited() || timedout) { throw new IllegalStateException("getTerminatingSignal() not defined"); } return waitResult - SIGNAL_1 + 1; @@ -139,16 +148,20 @@ * e.g. "Exit 1" or "Hangup". */ public String toShortString() { - return exited() - ? ("Exit " + getExitCode()) - : (getSignalString(getTerminatingSignal())); + return exited() ? "Exit " + getExitCode() + : timedout ? "Timeout" + : getSignalString(getTerminatingSignal()); } @Override public String toString() { - return exited() - ? ("Process exited with status " + getExitCode()) - : ("Process terminated by signal " + getTerminatingSignal()); + if (exited()) { + return "Process exited with status " + getExitCode(); + } else if (timedout) { + return "Timed out"; + } else { + return "Process terminated by signal " + getTerminatingSignal(); + } } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java index 6c5df1b..04da210 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
@@ -77,11 +77,11 @@ .getEventBus() .post(ActionStatusMessage.runningStrategy(spawn.getResourceOwner(), "standalone")); - int timeout = -1; + int timeoutSeconds = -1; String timeoutStr = spawn.getExecutionInfo().get("timeout"); if (timeoutStr != null) { try { - timeout = Integer.parseInt(timeoutStr); + timeoutSeconds = Integer.parseInt(timeoutStr); } catch (NumberFormatException e) { throw new UserExecException("could not parse timeout: ", e); } @@ -97,7 +97,7 @@ // Disable it for now to make the setup easier and to avoid further PATH hacks. // Ideally we should have a native implementation of process-wrapper for Windows. args.add(processWrapper.getPathString()); - args.add(Integer.toString(timeout)); + args.add(Integer.toString(timeoutSeconds)); args.add("5"); /* kill delay: give some time to print stacktraces and whatnot. */ // TODO(bazel-team): use process-wrapper redirection so we don't have to @@ -109,7 +109,8 @@ String cwd = executor.getExecRoot().getPathString(); Command cmd = new Command(args.toArray(new String[]{}), - locallyDeterminedEnv(spawn.getEnvironment()), new File(cwd)); + locallyDeterminedEnv(spawn.getEnvironment()), new File(cwd), + OS.getCurrent() == OS.WINDOWS && timeoutSeconds >= 0 ? timeoutSeconds * 1000 : -1); FileOutErr outErr = actionExecutionContext.getFileOutErr(); try { @@ -121,7 +122,8 @@ /*killSubprocessOnInterrupt*/ true); } catch (AbnormalTerminationException e) { TerminationStatus status = e.getResult().getTerminationStatus(); - boolean timedOut = !status.exited() && (status.getTerminatingSignal() == 14 /* SIGALRM */); + boolean timedOut = !status.exited() && ( + status.timedout() || status.getTerminatingSignal() == 14 /* SIGALRM */); String message = CommandFailureUtils.describeCommandFailure( verboseFailures, spawn.getArguments(), spawn.getEnvironment(), cwd);
diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java index 2188068..5774f0f 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java
@@ -76,11 +76,15 @@ static native int nativeReadStream(long stream, byte[] bytes, int offset, int length); /** - * Waits until the given process terminates. + * Waits until the given process terminates. If timeout is non-negative, it indicates the number + * of milliseconds before the call times out. * - * <p>Returns true if the process terminated or false if something went wrong. + * <p>Return values: + * <li>0: Process finished</li> + * <li>1: Timeout</li> + * <li>2: Something went wrong</li> */ - static native boolean nativeWaitFor(long process); + static native int nativeWaitFor(long process, long timeout); /** * Returns the exit code of the process. Throws {@code IllegalStateException} if something
diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java index 2457749..e0009d5 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.windows; import com.google.devtools.build.lib.shell.Subprocess; - import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -23,6 +22,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; /** @@ -124,12 +124,14 @@ private final ProcessInputStream stdoutStream; private final ProcessInputStream stderrStream; private final CountDownLatch waitLatch; + private final long timeoutMillis; + private final AtomicBoolean timedout = new AtomicBoolean(false); - - WindowsSubprocess( - long nativeProcess, String commandLine, boolean stdoutRedirected, boolean stderrRedirected) { + WindowsSubprocess(long nativeProcess, String commandLine, boolean stdoutRedirected, + boolean stderrRedirected, long timeoutMillis) { this.commandLine = commandLine; this.nativeProcess = nativeProcess; + this.timeoutMillis = timeoutMillis; stdoutStream = stdoutRedirected ? null @@ -150,11 +152,24 @@ } private void waiterThreadFunc() { - if (!WindowsProcesses.nativeWaitFor(nativeProcess)) { - // There isn't a lot we can do -- the process is still alive but WaitForMultipleObjects() - // failed for some odd reason. We'll pretend it terminated and log a message to jvm.out . - System.err.println("Waiting for process " - + WindowsProcesses.nativeGetProcessPid(nativeProcess) + " failed"); + switch (WindowsProcesses.nativeWaitFor(nativeProcess, timeoutMillis)) { + case 0: + // Excellent, process finished in time. + break; + + case 1: + // Timeout. Terminate the process if we can. + timedout.set(true); + WindowsProcesses.nativeTerminate(nativeProcess); + break; + + case 2: + // Error. There isn't a lot we can do -- the process is still alive but + // WaitForMultipleObjects() failed for some odd reason. We'll pretend it terminated and + // log a message to jvm.out . + System.err.println("Waiting for process " + + WindowsProcesses.nativeGetProcessPid(nativeProcess) + " failed"); + break; } waitLatch.countDown(); @@ -198,6 +213,11 @@ } @Override + public boolean timedout() { + return timedout.get(); + } + + @Override public void waitFor() throws InterruptedException { waitLatch.await(); }
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 10759a9..fbd0cdd 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
@@ -52,8 +52,8 @@ throw new IOException(error); } - return new WindowsSubprocess( - nativeProcess, commandLine, stdoutPath != null, stderrPath != null); + return new WindowsSubprocess(nativeProcess, commandLine, stdoutPath != null, + stderrPath != null, builder.getTimeoutMillis()); } private String getRedirectPath(StreamAction action, File file) {
diff --git a/src/main/native/windows_processes.cc b/src/main/native/windows_processes.cc index 5167857..3231c97 100644 --- a/src/main/native/windows_processes.cc +++ b/src/main/native/windows_processes.cc
@@ -412,21 +412,29 @@ return exit_code; } -extern "C" JNIEXPORT jboolean JNICALL +// return values: +// 0: Wait completed successfully +// 1: Timeout +// 2: Wait returned with an error +extern "C" JNIEXPORT jint JNICALL Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeWaitFor( - JNIEnv *env, jclass clazz, jlong process_long) { + JNIEnv *env, jclass clazz, jlong process_long, jlong java_timeout) { NativeProcess* process = reinterpret_cast<NativeProcess*>(process_long); HANDLE handles[1] = { process->process_ }; - switch (WaitForMultipleObjects(1, handles, FALSE, INFINITE)) { + DWORD win32_timeout = java_timeout < 0 ? INFINITE : java_timeout; + switch (WaitForMultipleObjects(1, handles, FALSE, win32_timeout)) { case 0: - return true; + return 0; + + case WAIT_TIMEOUT: + return 1; case WAIT_FAILED: - return false; + return 2; default: process->error_ = "WaitForMultipleObjects() returned unknown result"; - return false; + return 2; } }