Fix an issue where a "build" command might hang after it finished, because sandbox directories could not be cleaned up.
--
MOS_MIGRATED_REVID=134286101
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index 004c8df..de018b1 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -26,6 +26,8 @@
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
@@ -38,7 +40,10 @@
/** Helper methods that are shared by the different sandboxing strategies in this package. */
final class SandboxHelpers {
- static void lazyCleanup(ExecutorService backgroundWorkers, final SandboxRunner runner) {
+ static void lazyCleanup(
+ ExecutorService backgroundWorkers,
+ final EventHandler eventHandler,
+ final SandboxRunner runner) {
// By deleting the sandbox directory in the background, we avoid having to wait for it to
// complete before returning from the action, which improves performance.
backgroundWorkers.execute(
@@ -46,18 +51,15 @@
@Override
public void run() {
try {
- while (!Thread.currentThread().isInterrupted()) {
- try {
- runner.cleanup();
- return;
- } catch (IOException e2) {
- // Sleep & retry.
- Thread.sleep(250);
- }
- }
- } catch (InterruptedException e) {
- // Mark ourselves as interrupted and then exit.
- Thread.currentThread().interrupt();
+ runner.cleanup();
+ } catch (IOException e) {
+ // Can't do anything except logging here. SandboxModule#afterCommand will try again
+ // and alert the user if cleanup still fails.
+ eventHandler.handle(
+ Event.warn(
+ String.format(
+ "Could not delete sandbox directory after action execution: %s (%s)",
+ runner.getSandboxPath(), e)));
}
}
});
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
index 53c5c67..df28aaf 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
@@ -21,11 +21,13 @@
import com.google.devtools.build.lib.actions.ActionContextProvider;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.buildtool.buildevent.BuildStartingEvent;
-import com.google.devtools.build.lib.concurrent.ExecutorUtil;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.OptionsBase;
import java.io.IOException;
import java.util.concurrent.ExecutorService;
@@ -35,18 +37,18 @@
* This module provides the Sandbox spawn strategy.
*/
public final class SandboxModule extends BlazeModule {
- // Per-server state
- private ExecutorService backgroundWorkers;
-
// Per-command state
private CommandEnvironment env;
private BuildRequest buildRequest;
+ private ExecutorService backgroundWorkers;
+ private SandboxOptions sandboxOptions;
@Override
public Iterable<ActionContextProvider> getActionContextProviders() {
Preconditions.checkNotNull(env);
Preconditions.checkNotNull(buildRequest);
Preconditions.checkNotNull(backgroundWorkers);
+ sandboxOptions = buildRequest.getOptions(SandboxOptions.class);
try {
return ImmutableList.<ActionContextProvider>of(
SandboxActionContextProvider.create(env, buildRequest, backgroundWorkers));
@@ -79,13 +81,46 @@
@Override
public void afterCommand() {
+ // We want to make sure that all sandbox directories are deleted after a command finishes or at
+ // least the user gets notified if some of them can't be deleted. However we can't rely on the
+ // background workers for that, because a) they can't log, and b) if a directory is undeletable,
+ // the Runnable might never finish. So we cancel them and delete the remaining directories here,
+ // where we have more control.
+ backgroundWorkers.shutdownNow();
+ if (sandboxOptions != null && !sandboxOptions.sandboxDebug) {
+ Path sandboxRoot =
+ env.getDirectories()
+ .getOutputBase()
+ .getRelative(env.getRuntime().getProductName() + "-sandbox");
+ if (sandboxRoot.exists()) {
+ try {
+ for (Path child : sandboxRoot.getDirectoryEntries()) {
+ try {
+ FileSystemUtils.deleteTree(child);
+ } catch (IOException e) {
+ env.getReporter()
+ .handle(
+ Event.warn(
+ String.format(
+ "Could not delete sandbox directory: %s (%s)",
+ child.getPathString(), e)));
+ }
+ }
+ sandboxRoot.delete();
+ } catch (IOException e) {
+ env.getReporter()
+ .handle(
+ Event.warn(
+ String.format(
+ "Could not delete %s directory: %s", sandboxRoot.getBaseName(), e)));
+ }
+ }
+ }
+
env = null;
buildRequest = null;
-
- // "bazel clean" will also try to delete the sandbox directories, leading to a race condition
- // if it is run right after a "bazel build". We wait for and shutdown the background worker pool
- // before continuing to avoid this.
- ExecutorUtil.interruptibleShutdown(backgroundWorkers);
+ backgroundWorkers = null;
+ sandboxOptions = null;
}
@Subscribe
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxRunner.java
index 4bb2b95..dabbf66 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxRunner.java
@@ -122,4 +122,8 @@
FileSystemUtils.deleteTree(sandboxPath);
}
}
+
+ Path getSandboxPath() {
+ return sandboxPath;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
index d02ffc2..b5d787c 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -70,6 +71,7 @@
SandboxRunner runner,
AtomicReference<Class<? extends SpawnActionContext>> writeOutputFiles)
throws ExecException, InterruptedException {
+ EventHandler eventHandler = actionExecutionContext.getExecutor().getEventHandler();
try {
runner.run(
spawn.getArguments(),
@@ -90,17 +92,14 @@
} catch (IOException e) {
// Catch the IOException and turn it into an error message, otherwise this might hide an
// exception thrown during runner.run earlier.
- actionExecutionContext
- .getExecutor()
- .getEventHandler()
- .handle(
- Event.error(
- "I/O exception while extracting output artifacts from sandboxed execution: "
- + e));
+ eventHandler.handle(
+ Event.error(
+ "I/O exception while extracting output artifacts from sandboxed execution: "
+ + e));
}
}
if (!sandboxOptions.sandboxDebug) {
- SandboxHelpers.lazyCleanup(backgroundWorkers, runner);
+ SandboxHelpers.lazyCleanup(backgroundWorkers, eventHandler, runner);
}
}
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
index 38b9189..17e3d4f 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -222,8 +222,7 @@
bazel build examples/genrule:tools_work &> $TEST_log \
|| fail "Hermetic genrule failed: examples/genrule:tools_work"
bazel shutdown &> $TEST_log || fail "bazel shutdown failed"
- ls -la "$(bazel info output_base)/bazel-sandbox"
- if [[ "$(ls -A "$(bazel info execution_root)"/bazel-sandbox)" ]]; then
+ if [[ "$(ls -la "$(bazel info output_base)/bazel-sandbox")" ]]; then
fail "Build left files around afterwards"
fi
}
