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;
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/windows/MockSubprocess.java b/src/test/java/com/google/devtools/build/lib/windows/MockSubprocess.java
index 1562532..540162c 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/MockSubprocess.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/MockSubprocess.java
@@ -34,6 +34,7 @@
* <li><code>O<register></code>: Write the contents of a register to stdout</li>
* <li><code>E<register></code>: Write the contents of a register to stderr</li>
* <li><code>X<exit code%gt;</code>: Exit with the specified exit code</li>
+ * <li><code>S<seconds></code>: Wait the specified number of seconds</li>
* </ul>
*
* <p>Registers are single characters. Each command line argument is interpreted as a single
@@ -95,6 +96,15 @@
writeBytes(System.out, arg);
break;
+ case 'W':
+ try {
+ Thread.sleep(Integer.parseInt(arg.substring(1)) * 1000);
+ } catch (InterruptedException e) {
+ // This is good enough for a mock process
+ throw new IllegalStateException(e);
+ }
+ break;
+
case 'X':
System.exit(Integer.parseInt(arg.substring(1)));
}
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
index 074c81d..0b09216 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
@@ -133,7 +133,7 @@
@Test
public void testExitCode() throws Exception {
process = WindowsProcesses.nativeCreateProcess(mockArgs("X42"), null, null, null, null);
- assertThat(WindowsProcesses.nativeWaitFor(process)).isTrue();
+ assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0);
assertThat(WindowsProcesses.nativeGetExitCode(process)).isEqualTo(42);
assertNoProcessError();
}
@@ -302,7 +302,7 @@
null, null, stdoutFile, stderrFile);
assertThat(process).isGreaterThan(0L);
assertNoProcessError();
- assertThat(WindowsProcesses.nativeWaitFor(process)).isTrue();
+ assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0);
WindowsProcesses.nativeGetExitCode(process);
assertNoProcessError();
byte[] stdout = Files.readAllBytes(Paths.get(stdoutFile));
@@ -319,7 +319,7 @@
null, null, file, file);
assertThat(process).isGreaterThan(0L);
assertNoProcessError();
- assertThat(WindowsProcesses.nativeWaitFor(process)).isTrue();
+ assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0);
WindowsProcesses.nativeGetExitCode(process);
assertNoProcessError();
byte[] bytes = Files.readAllBytes(Paths.get(file));
@@ -337,7 +337,7 @@
byte[] buf = new byte[1];
assertThat(readStdout(buf, 0, 1)).isEqualTo(-1);
assertThat(readStderr(buf, 0, 1)).isEqualTo(-1);
- WindowsProcesses.nativeWaitFor(process);
+ WindowsProcesses.nativeWaitFor(process, -1);
}
@Test
@@ -352,7 +352,7 @@
process = WindowsProcesses.nativeCreateProcess(mockArgs("O-out2", "E-err2"), null,
null, stdoutFile, stderrFile);
assertNoProcessError();
- WindowsProcesses.nativeWaitFor(process);
+ WindowsProcesses.nativeWaitFor(process, -1);
WindowsProcesses.nativeGetExitCode(process);
assertNoProcessError();
byte[] stdoutBytes = Files.readAllBytes(Paths.get(stdoutFile));
@@ -372,6 +372,11 @@
int len = readStdout(buf, 0, 1024);
assertNoProcessError();
assertThat(new String(buf, 0, len, UTF8).replace("\\", "/")).isEqualTo(dir1);
+ }
+ @Test
+ public void testTimeout() throws Exception {
+ process = WindowsProcesses.nativeCreateProcess(mockArgs("W5", "X0"), null, null, null, null);
+ assertThat(WindowsProcesses.nativeWaitFor(process, 1000)).isEqualTo(1);
}
}
