Wait for process termination when the LocalSpawnRunner is interrupted. The dynamic scheduler cancels the futures it uses to run spawns, which causes the spawns executed via the LocalSpawnRunner to be interrupted. Once interrupted, the spawn runner kills the subprocess... but it was not waiting for the subprocess' termination. This could lead to races because signal delivery is not synchronous and the subprocess might continue running for a little bit while the dynamic scheduler decides to do something else with the other (remote) spawn. I haven't observed these races in the wild, but I'm seeing problems while modifying the dynamic scheduler to forcibly cancel local spawns once we have scored a cache hit. To fix this problem, make the LocalSpawnRunner wait for process termination after it forcibly destroys the subprocess on interrupt. And, while doing so, homogenize all (?) the places where we try to forcibly terminate subprocesses. Addresses issue #7818. RELNOTES: None. PiperOrigin-RevId: 268724818
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java index f0fe80a..dd6d1ec 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -372,7 +372,7 @@ terminationStatus = new TerminationStatus(subprocess.exitValue(), subprocess.timedout()); } catch (InterruptedException e) { - subprocess.destroy(); + subprocess.destroyAndWait(); throw e; } } catch (IOException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java index 3bf345a..3f30c71 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
@@ -232,8 +232,7 @@ // TODO(jmmv): This is adapted from Worker.java. Should probably replace both with a new variant // of Uninterruptibles.callUninterruptibly that takes a lambda instead of a callable. private static void destroyProcess(Subprocess process) { - process.destroy(); - SandboxHelpers.waitForProcess(process); + process.destroyAndWait(); } @Override
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 f058a9a..8cb0988 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
@@ -71,4 +71,29 @@ */ @Override void close(); + + /** + * Kills the subprocess and awaits for its termination so that we know it has released any + * resources it may have held. + */ + default void destroyAndWait() { + destroy(); + + boolean wasInterrupted = false; + try { + while (true) { + try { + waitFor(); + return; + } catch (InterruptedException ie) { + wasInterrupted = true; + } + } + } finally { + // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/ + if (wasInterrupted) { + Thread.currentThread().interrupt(); // preserve interrupted status + } + } + } }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java index e77bb35..8c39925 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
@@ -89,33 +89,7 @@ Runtime.getRuntime().removeShutdownHook(shutdownHook); } if (process != null) { - destroyProcess(process); - } - } - - /** - * Destroys a process and waits for it to exit. This is necessary for the child to not become a - * zombie. - * - * @param process the process to destroy. - */ - private static void destroyProcess(Subprocess process) { - boolean wasInterrupted = false; - try { - process.destroy(); - while (true) { - try { - process.waitFor(); - return; - } catch (InterruptedException ie) { - wasInterrupted = true; - } - } - } finally { - // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/ - if (wasInterrupted) { - Thread.currentThread().interrupt(); // preserve interrupted status - } + process.destroyAndWait(); } }
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 99ba49b..d4daedf 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
@@ -26,6 +26,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.google.common.io.Files; import com.google.devtools.build.lib.actions.ActionInput; @@ -61,6 +62,7 @@ import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.JavaIoFileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; @@ -71,12 +73,17 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.Semaphore; import java.util.concurrent.ThreadLocalRandom; import java.util.logging.Filter; import java.util.logging.LogRecord; @@ -637,6 +644,92 @@ } @Test + public void interruptWaitsForProcessExit() throws Exception { + assumeTrue(OS.getCurrent() != OS.WINDOWS); + + File tempDirFile = TestUtils.makeTempDir(); + tempDirFile.deleteOnExit(); + FileSystem fs = new JavaIoFileSystem(DigestHashFunction.getDefaultUnchecked()); + Path tempDir = fs.getPath(tempDirFile.getPath()); + + LocalSpawnRunner runner = + new LocalSpawnRunner( + tempDir, + Options.getDefaults(LocalExecutionOptions.class), + resourceManager, + /*useProcessWrapper=*/ false, + OS.LINUX, + LocalEnvProvider.forCurrentOs(ImmutableMap.of()), + /*binTools=*/ null, + Mockito.mock(RunfilesTreeUpdater.class)); + FileOutErr fileOutErr = + new FileOutErr(tempDir.getRelative("stdout"), tempDir.getRelative("stderr")); + SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); + + // This test to exercise a race condition by attempting an operation multiple times. We can get + // false positives (the test passing without us catching a problem), so try a few times. When + // implementing this fix on 2019-09-11, this specific configuration was sufficient to catch the + // previously-existent bug. + int tries = 10; + int delaySeconds = 1; + + Path content = tempDir.getChild("content"); + Path started = tempDir.getChild("started"); + // Start a subprocess that blocks until it is killed, and when it is, writes some output to + // a temporary file after some delay. + String script = + "trap 'sleep " + + delaySeconds + + "; echo foo >" + + content.getPathString() + + "; exit 1' TERM; " + + "touch " + + started.getPathString() + + "; " + + "while :; do " + + " echo 'waiting to be killed'; " + + " sleep 1; " + + "done"; + Spawn spawn = new SpawnBuilder("/bin/sh", "-c", script).build(); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + for (int i = 0; i < tries; i++) { + content.delete(); + started.delete(); + Semaphore interruptCaught = new Semaphore(0); + Future<?> future = + executor.submit( + () -> { + try { + runner.exec(spawn, policy); + } catch (InterruptedException e) { + interruptCaught.release(); + } catch (Throwable t) { + throw new IllegalStateException(t); + } + }); + // Wait until we know the subprocess has started so that delivering a termination signal + // to it triggers the delayed write to the file. + while (!started.exists()) { + Thread.sleep(1); + } + future.cancel(true); + interruptCaught.acquireUninterruptibly(); + // At this point, the subprocess must have fully stopped so write some content to the file + // and expect that these contents remain unmodified. + FileSystemUtils.writeContent(content, StandardCharsets.UTF_8, "bar"); + // Wait for longer than the spawn takes to exit before we check the file contents to ensure + // that we properly awaited for termination of the subprocess. + Thread.sleep(delaySeconds * 2 * 1000); + assertThat(FileSystemUtils.readContent(content, StandardCharsets.UTF_8)).isEqualTo("bar"); + } + } finally { + executor.shutdown(); + } + } + + @Test public void checkPrefetchCalled() throws Exception { FileSystem fs = setupEnvironmentForFakeExecution();