Skip to content

Commit

Permalink
sandbox: Replace the error-prone lazy cleanup of sandbox directories …
Browse files Browse the repository at this point in the history
…by a simple synchronous cleanup.

Tested with bazel building itself that this does not result in a performance degradation.

--
MOS_MIGRATED_REVID=134766597
  • Loading branch information
philwo authored and meteorcloudy committed Sep 30, 2016
1 parent c470ae7 commit 95b16a8
Show file tree
Hide file tree
Showing 10 changed files with 58 additions and 135 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ final class DarwinSandboxRunner extends SandboxRunner {
Set<Path> inaccessiblePaths,
Path runUnderPath,
boolean verboseFailures) {
super(sandboxPath, sandboxExecRoot, verboseFailures);
super(sandboxExecRoot, verboseFailures);
this.sandboxExecRoot = sandboxExecRoot;
this.argumentsFilePath = sandboxPath.getRelative("sandbox.sb");
this.writableDirs = writableDirs;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.config.RunUnder;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.rules.test.TestRunnerAction;
import com.google.devtools.build.lib.shell.Command;
import com.google.devtools.build.lib.shell.CommandException;
Expand All @@ -51,7 +52,6 @@
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

Expand All @@ -78,15 +78,13 @@ private DarwinSandboxedStrategy(
BuildRequest buildRequest,
Map<String, String> clientEnv,
BlazeDirectories blazeDirs,
ExecutorService backgroundWorkers,
boolean verboseFailures,
String productName,
ImmutableList<Path> confPaths,
SpawnHelpers spawnHelpers) {
super(
buildRequest,
blazeDirs,
backgroundWorkers,
verboseFailures,
buildRequest.getOptions(SandboxOptions.class));
this.clientEnv = ImmutableMap.copyOf(clientEnv);
Expand All @@ -103,7 +101,6 @@ public static DarwinSandboxedStrategy create(
BuildRequest buildRequest,
Map<String, String> clientEnv,
BlazeDirectories blazeDirs,
ExecutorService backgroundWorkers,
boolean verboseFailures,
String productName)
throws IOException {
Expand All @@ -122,7 +119,6 @@ public static DarwinSandboxedStrategy create(
buildRequest,
clientEnv,
blazeDirs,
backgroundWorkers,
verboseFailures,
productName,
writablePaths.build(),
Expand Down Expand Up @@ -213,14 +209,30 @@ public void exec(
getInaccessiblePaths(),
runUnderPath,
verboseFailures);
runSpawn(
spawn,
actionExecutionContext,
spawnEnvironment,
hardlinkedExecRoot,
outputs,
runner,
writeOutputFiles);
try {
runSpawn(
spawn,
actionExecutionContext,
spawnEnvironment,
hardlinkedExecRoot,
outputs,
runner,
writeOutputFiles);
} finally {
if (!sandboxDebug) {
try {
FileSystemUtils.deleteTree(sandboxPath);
} catch (IOException e) {
executor
.getEventHandler()
.handle(
Event.error(
String.format(
"Cannot delete sandbox directory after action execution: %s (%s)",
sandboxPath.getPathString(), e)));
}
}
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ final class LinuxSandboxRunner extends SandboxRunner {
Set<Path> bindMounts,
boolean verboseFailures,
boolean sandboxDebug) {
super(sandboxPath, sandboxExecRoot, verboseFailures);
super(sandboxExecRoot, verboseFailures);
this.execRoot = execRoot;
this.sandboxExecRoot = sandboxExecRoot;
this.sandboxTempDir = sandboxTempDir;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@
import com.google.devtools.build.lib.actions.UserExecException;
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.runtime.CommandEnvironment;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

Expand Down Expand Up @@ -63,14 +64,12 @@ public static boolean isSupported(CommandEnvironment env) {
LinuxSandboxedStrategy(
BuildRequest buildRequest,
BlazeDirectories blazeDirs,
ExecutorService backgroundWorkers,
boolean verboseFailures,
String productName,
boolean fullySupported) {
super(
buildRequest,
blazeDirs,
backgroundWorkers,
verboseFailures,
buildRequest.getOptions(SandboxOptions.class));
this.sandboxOptions = buildRequest.getOptions(SandboxOptions.class);
Expand Down Expand Up @@ -123,14 +122,30 @@ public void exec(
}

SandboxRunner runner = getSandboxRunner(spawn, sandboxPath, sandboxExecRoot, sandboxTempDir);
runSpawn(
spawn,
actionExecutionContext,
spawn.getEnvironment(),
symlinkedExecRoot,
outputs,
runner,
writeOutputFiles);
try {
runSpawn(
spawn,
actionExecutionContext,
spawn.getEnvironment(),
symlinkedExecRoot,
outputs,
runner,
writeOutputFiles);
} finally {
if (!sandboxOptions.sandboxDebug) {
try {
FileSystemUtils.deleteTree(sandboxPath);
} catch (IOException e) {
executor
.getEventHandler()
.handle(
Event.error(
String.format(
"Cannot delete sandbox directory after action execution: %s (%s)",
sandboxPath.getPathString(), e)));
}
}
}
}

private SandboxRunner getSandboxRunner(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ final class ProcessWrapperRunner extends SandboxRunner {

ProcessWrapperRunner(
Path execRoot, Path sandboxPath, Path sandboxExecRoot, boolean verboseFailures) {
super(sandboxPath, sandboxExecRoot, verboseFailures);
super(sandboxExecRoot, verboseFailures);
this.execRoot = execRoot;
this.sandboxExecRoot = sandboxExecRoot;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.util.OS;
import java.io.IOException;
import java.util.concurrent.ExecutorService;

/**
* Provides the sandboxed spawn strategy.
Expand All @@ -43,8 +42,7 @@ private SandboxActionContextProvider(ImmutableList<ActionContext> contexts) {
}

public static SandboxActionContextProvider create(
CommandEnvironment env, BuildRequest buildRequest, ExecutorService backgroundWorkers)
throws IOException {
CommandEnvironment env, BuildRequest buildRequest) throws IOException {
boolean verboseFailures = buildRequest.getOptions(ExecutionOptions.class).verboseFailures;
ImmutableList.Builder<ActionContext> contexts = ImmutableList.builder();

Expand All @@ -60,7 +58,6 @@ public static SandboxActionContextProvider create(
new LinuxSandboxedStrategy(
buildRequest,
env.getDirectories(),
backgroundWorkers,
verboseFailures,
env.getRuntime().getProductName(),
fullySupported));
Expand All @@ -73,7 +70,6 @@ public static SandboxActionContextProvider create(
buildRequest,
env.getClientEnv(),
env.getDirectories(),
backgroundWorkers,
verboseFailures,
env.getRuntime().getProductName()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,45 +26,16 @@
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;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.atomic.AtomicInteger;

/** Helper methods that are shared by the different sandboxing strategies in this package. */
public final class SandboxHelpers {

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(
new Runnable() {
@Override
public void run() {
try {
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)));
}
}
});
}

static void fallbackToNonSandboxedExecution(
Spawn spawn, ActionExecutionContext actionExecutionContext, Executor executor)
throws ExecException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,16 @@

import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.Subscribe;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.actions.ActionContextConsumer;
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.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;
import java.util.concurrent.Executors;

/**
* This module provides the Sandbox spawn strategy.
Expand All @@ -40,18 +34,14 @@ public final class SandboxModule extends BlazeModule {
// 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));
SandboxActionContextProvider.create(env, buildRequest));
} catch (IOException e) {
throw new IllegalStateException(e);
}
Expand All @@ -72,55 +62,14 @@ public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command)

@Override
public void beforeCommand(Command command, CommandEnvironment env) {
backgroundWorkers =
Executors.newCachedThreadPool(
new ThreadFactoryBuilder().setNameFormat("sandbox-background-worker-%d").build());
this.env = env;
env.getEventBus().register(this);
}

@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;
backgroundWorkers = null;
sandboxOptions = null;
}

@Subscribe
Expand Down
Loading

0 comments on commit 95b16a8

Please sign in to comment.