diff options
author | ulfjack <ulfjack@google.com> | 2017-07-14 17:39:46 +0200 |
---|---|---|
committer | Jakob Buchgraber <buchgr@google.com> | 2017-07-17 10:10:47 +0200 |
commit | 9f18a511205fbd74e188f0d42974fe329b9cffe6 (patch) | |
tree | f93c18285bbbed5d59efbe6341fe6ab006b4822b | |
parent | 33bd2570b7e942eaf13145c66adc53f223c0b80b (diff) |
Simplify exception handling in spawn strategies
The main change here is to only catch SpawnExecException in
StandaloneTestStrategy, so all other exceptions simplify propagate up. As a
result, Bazel no longer retries tests that fail with an exception, we only
retry tests that actually ran, had a spawn result, and resulted in a
UserExecException. That is probably what we want.
Also do some cleanup:
- Remove ExecException.timedOut; nobody was calling it (but there's still
SpawnExecException.timedOut)
- Remove SpawnActionContext.shouldPropagateExecException; all exceptions
(except SpawnExecException) are now propagated by default
- Remote the SandboxOptions from the SandboxStrategies; all sandboxing options
are now handled by the underlying SpawnRunner implementations
I'll send a followup CL to remove the UserExecException and
EnvironmentalExecException types; the types don't do anything special, and
there are no catch blocks in production code that catch one of these more
specific types.
This should fix #3322 by removing a bunch of special handling.
PiperOrigin-RevId: 161960919
11 files changed, 5 insertions, 48 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java index 16f4550195..a217ac259b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java @@ -98,12 +98,4 @@ public abstract class ExecException extends Exception { */ public abstract ActionExecutionException toActionExecutionException(String messagePrefix, boolean verboseFailures, Action action); - - - /** - * Tells if the execution exception was thrown because of the execution timing out. - */ - public boolean hasTimedOut() { - return false; - } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java index b57d4e255b..e37887ffd8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java @@ -22,11 +22,4 @@ public interface SpawnActionContext extends ActionContext { /** Executes the given spawn. */ void exec(Spawn spawn, ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException; - - /** - * If an ExecException should be rethrown by the strategy that executed this. - * Currently only works for LinuxSandboxedStrategy: - * If true, will throw ExecException and give reproduction instruction for sandbox. - */ - boolean shouldPropagateExecException(); } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java index 4c9bd95372..bd9f5d8485 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java @@ -49,7 +49,6 @@ public class SpawnExecException extends ExecException { return result; } - @Override public boolean hasTimedOut() { return getSpawnResult().status() == Status.TIMEOUT; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 8ef6049ae8..acbf5c4963 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -325,15 +325,14 @@ public class StandaloneTestStrategy extends TestStrategy { .setTestPassed(true) .setStatus(BlazeTestStatus.PASSED) .setPassedLog(testLogPath.getPathString()); - } catch (ExecException e) { - // Execution failed, which we consider a test failure. + } catch (SpawnExecException e) { + // If this method returns normally, then the higher level will rerun the test (up to + // --flaky_test_attempts times). We don't catch any other ExecException here, so those never + // get retried. builder .setTestPassed(false) .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED) .addFailedLogs(testLogPath.getPathString()); - if (spawnActionContext.shouldPropagateExecException()) { - throw e; - } } finally { long duration = actionExecutionContext.getClock().currentTimeMillis() - startTime; builder.setStartTimeMillisEpoch(startTime); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java index 87c00ee5c6..44d62b6493 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java @@ -167,9 +167,4 @@ final class RemoteSpawnStrategy implements SpawnActionContext { throw new SpawnExecException(message, result, /*catastrophe=*/ false); } } - - @Override - public boolean shouldPropagateExecException() { - return false; - } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java index 38c8645f2c..29b572fe53 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java @@ -36,7 +36,6 @@ final class DarwinSandboxedStrategy extends SandboxStrategy { String productName) throws IOException { super( verboseFailures, - buildRequest.getOptions(SandboxOptions.class), new DarwinSandboxedSpawnRunner(cmdEnv, buildRequest, sandboxBase, productName)); } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java index 3d556d3bfc..8b7b2b7fdd 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java @@ -38,7 +38,6 @@ public final class LinuxSandboxedStrategy extends SandboxStrategy { Path inaccessibleHelperDir) { super( verboseFailures, - buildRequest.getOptions(SandboxOptions.class), new LinuxSandboxedSpawnRunner( cmdEnv, buildRequest, sandboxBase, inaccessibleHelperFile, inaccessibleHelperDir)); } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java index 8c770a8bb2..1e59735a0f 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java @@ -35,7 +35,6 @@ final class ProcessWrapperSandboxedStrategy extends SandboxStrategy { String productName) { super( verboseFailures, - buildRequest.getOptions(SandboxOptions.class), new ProcessWrapperSandboxedSpawnRunner(cmdEnv, buildRequest, sandboxBase, productName)); } } 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 f290b812c1..7b325f61dc 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 @@ -43,16 +43,13 @@ import java.util.concurrent.atomic.AtomicReference; /** Abstract common ancestor for sandbox strategies implementing the common parts. */ abstract class SandboxStrategy implements SandboxedSpawnActionContext { private final boolean verboseFailures; - private final SandboxOptions sandboxOptions; private final SpawnInputExpander spawnInputExpander; private final AbstractSandboxSpawnRunner spawnRunner; public SandboxStrategy( boolean verboseFailures, - SandboxOptions sandboxOptions, AbstractSandboxSpawnRunner spawnRunner) { this.verboseFailures = verboseFailures; - this.sandboxOptions = sandboxOptions; this.spawnInputExpander = new SpawnInputExpander(false); this.spawnRunner = spawnRunner; } @@ -141,7 +138,7 @@ abstract class SandboxStrategy implements SandboxedSpawnActionContext { if (result.status() != Status.SUCCESS || result.exitCode() != 0) { String message = CommandFailureUtils.describeCommandFailure( - true, spawn.getArguments(), spawn.getEnvironment(), null); + verboseFailures, spawn.getArguments(), spawn.getEnvironment(), null); throw new SpawnExecException( message, result, /*forciblyRunRemotely=*/false, /*catastrophe=*/false); } @@ -151,9 +148,4 @@ abstract class SandboxStrategy implements SandboxedSpawnActionContext { public String toString() { return "sandboxed"; } - - @Override - public boolean shouldPropagateExecException() { - return verboseFailures && sandboxOptions.sandboxDebug; - } } 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 b1e0e9d93b..4a637d0001 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 @@ -154,9 +154,4 @@ public class StandaloneSpawnStrategy implements SpawnActionContext { public String toString() { return "standalone"; } - - @Override - public boolean shouldPropagateExecException() { - return false; - } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java index 595751c124..5688933b94 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java @@ -397,9 +397,4 @@ public final class WorkerSpawnStrategy implements SandboxedSpawnActionContext { public String toString() { return "worker"; } - - @Override - public boolean shouldPropagateExecException() { - return false; - } } |