Refactor SpawnContinuation semantics
By making it safe to call execute and getFuture at all times, the client
code becomes much simpler and less brittle. We only need to make sure to
only call get() if isDone() returns true, but it's fine to call execute
even if the future is done.
Progress on #6394.
PiperOrigin-RevId: 268684800
diff --git a/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java
index 0534116..4a0c156 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
-import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
import com.google.devtools.build.lib.actions.RunningActionEvent;
import com.google.devtools.build.lib.actions.SpawnContinuation;
@@ -49,8 +48,7 @@
ActionExecutionContext actionExecutionContext,
DeterministicWriter deterministicWriter,
boolean makeExecutable,
- boolean isRemotable)
- throws ExecException {
+ boolean isRemotable) {
actionExecutionContext.getEventHandler().post(new RunningActionEvent(action, "local"));
// TODO(ulfjack): Consider acquiring local resources here before trying to write the file.
try (AutoProfiler p =
@@ -68,7 +66,7 @@
outputPath.setExecutable(true);
}
} catch (IOException e) {
- throw new EnvironmentalExecException(e);
+ return SpawnContinuation.failedWithExecException(new EnvironmentalExecException(e));
}
}
return SpawnContinuation.immediate();
diff --git a/src/main/java/com/google/devtools/build/lib/exec/ProxySpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/exec/ProxySpawnActionContext.java
index 2f65ec6..ab62b2d 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/ProxySpawnActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/ProxySpawnActionContext.java
@@ -50,10 +50,14 @@
@Override
public SpawnContinuation beginExecution(
- Spawn spawn, ActionExecutionContext actionExecutionContext)
- throws ExecException, InterruptedException {
- return resolveOne(spawn, actionExecutionContext.getEventHandler())
- .beginExecution(spawn, actionExecutionContext);
+ Spawn spawn, ActionExecutionContext actionExecutionContext) throws InterruptedException {
+ SpawnActionContext resolvedContext;
+ try {
+ resolvedContext = resolveOne(spawn, actionExecutionContext.getEventHandler());
+ } catch (ExecException e) {
+ return SpawnContinuation.failedWithExecException(e);
+ }
+ return resolvedContext.beginExecution(spawn, actionExecutionContext);
}
private SpawnActionContext resolveOne(Spawn spawn, EventHandler eventHandler)
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 b86c305..59ab2d8 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
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
@@ -285,7 +286,7 @@
Spawn spawn,
ActionExecutionContext actionExecutionContext,
Path execRoot)
- throws ExecException, IOException, InterruptedException {
+ throws IOException, InterruptedException {
ResolvedPaths resolvedPaths = testAction.resolve(execRoot);
Path out = actionExecutionContext.getInputPath(testAction.getTestLog());
Path err = resolvedPaths.getTestStderr();
@@ -297,17 +298,19 @@
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
}
long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
+ SpawnContinuation spawnContinuation =
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
return new BazelTestAttemptContinuation(
- testAction,
- actionExecutionContext,
- spawn,
- resolvedPaths,
- testOutErr,
- streamed,
- startTimeMillis,
- SpawnContinuation.ofBeginExecution(
- spawn, actionExecutionContext.withFileOutErr(testOutErr)))
- .execute();
+ testAction,
+ actionExecutionContext,
+ spawn,
+ resolvedPaths,
+ testOutErr,
+ streamed,
+ startTimeMillis,
+ spawnContinuation);
}
/** In rare cases, we might write something to stderr. Append it to the real test.log. */
@@ -439,8 +442,7 @@
}
@Override
- public TestAttemptContinuation beginExecution()
- throws InterruptedException, IOException, ExecException {
+ public TestAttemptContinuation beginExecution() throws InterruptedException, IOException {
prepareFileSystem(testAction, actionExecutionContext.getExecRoot(), tmpDir, workingDirectory);
return beginTestAttempt(testAction, spawn, actionExecutionContext, execRoot);
}
@@ -502,8 +504,7 @@
}
@Override
- public TestAttemptContinuation execute()
- throws InterruptedException, IOException, ExecException {
+ public TestAttemptContinuation execute() throws InterruptedException, ExecException {
// We have two protos to represent test attempts:
// 1. com.google.devtools.build.lib.view.test.TestStatus.TestResultData represents both failed
// attempts and finished tests. Bazel stores this to disk to persist cached test result
@@ -552,15 +553,19 @@
}
long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
- if (!fileOutErr.hasRecordedOutput()) {
- // Make sure that the test.log exists.
- FileSystemUtils.touchFile(fileOutErr.getOutputPath());
- }
- // Append any error output to the test.log. This is very rare.
- appendStderr(fileOutErr);
- fileOutErr.close();
- if (streamed != null) {
- streamed.close();
+ try {
+ if (!fileOutErr.hasRecordedOutput()) {
+ // Make sure that the test.log exists.
+ FileSystemUtils.touchFile(fileOutErr.getOutputPath());
+ }
+ // Append any error output to the test.log. This is very rare.
+ appendStderr(fileOutErr);
+ fileOutErr.close();
+ if (streamed != null) {
+ streamed.close();
+ }
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(e);
}
// SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there
@@ -595,21 +600,16 @@
// We treat all failures to generate the test.xml here as catastrophic, and won't rerun
// the test if this fails. We redirect the output to a temporary file.
FileOutErr xmlSpawnOutErr = actionExecutionContext.getFileOutErr().childOutErr();
- SpawnContinuation xmlContinuation;
try {
- xmlContinuation =
+ SpawnContinuation xmlContinuation =
spawnActionContext.beginExecution(
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
- } catch (ExecException | InterruptedException e) {
- xmlSpawnOutErr.close();
- throw e;
- }
- if (!xmlContinuation.isDone()) {
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, builder, spawnResults, xmlContinuation);
+ } catch (InterruptedException e) {
+ closeSuppressed(e, xmlSpawnOutErr);
+ throw e;
}
- spawnResults = new ArrayList<>(spawnResults);
- spawnResults.addAll(xmlContinuation.get());
}
TestCase details = parseTestResult(xmlOutputPath);
@@ -660,8 +660,7 @@
}
@Override
- public TestAttemptContinuation execute()
- throws InterruptedException, IOException, ExecException {
+ public TestAttemptContinuation execute() throws InterruptedException, ExecException {
SpawnContinuation nextContinuation;
try {
nextContinuation = spawnContinuation.execute();
