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/actions/SpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
index 7098390..0c71630 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
@@ -36,9 +36,12 @@
* the execution. Otherwise all requirements from {@link #exec} apply.
*/
default SpawnContinuation beginExecution(
- Spawn spawn, ActionExecutionContext actionExecutionContext)
- throws ExecException, InterruptedException {
- return SpawnContinuation.immediate(exec(spawn, actionExecutionContext));
+ Spawn spawn, ActionExecutionContext actionExecutionContext) throws InterruptedException {
+ try {
+ return SpawnContinuation.immediate(exec(spawn, actionExecutionContext));
+ } catch (ExecException e) {
+ return SpawnContinuation.failedWithExecException(e);
+ }
}
/** Returns whether this SpawnActionContext supports executing the given Spawn. */
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java
index 380cb4d..467b946 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.actions;
import com.google.common.collect.ImmutableList;
+import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import java.util.List;
@@ -25,14 +26,17 @@
* <p>This is intentionally similar to {@link ActionContinuationOrResult}, which will often wrap one
* of these.
*
- * <p>Any client of this class <b>must</b> first call {@link #isDone} before calling any of the
- * other methods. If {@link #isDone} returns true, then {@link #getFuture} and {@link #execute} must
- * throw {@link IllegalStateException}, but {@link #get} must return a valid value. Use {@link
- * #immediate} to construct such an instance.
+ * <p>Any client of this class <b>should</b> first call {@link #isDone} before calling any of the
+ * other methods. If {@link #isDone} returns true, then {@link #execute} must return this, and
+ * {@link #get} must return the result. Use {@link #immediate} to construct such an instance. {@link
+ * #getFuture} should return a completed future (see {@link Futures#immediateFuture}).
*
- * <p>Otherwise, {@link #getFuture} must return a non-null value, and {@link #execute} must not
- * throw {@link IllegalStateException}, whereas {@link #get} must throw {@link
- * IllegalStateException}.
+ * <p>Otherwise, {@link #getFuture} must return a non-null value, {@link #execute} must not throw
+ * {@link IllegalStateException}, and {@link #get} must throw {@link IllegalStateException}. Note
+ * that calling {@link #execute} without waiting for the future to complete will likely block.
+ *
+ * <p>That is, it is always safe to call {@link #isDone}, {@link #getFuture}, and {@link #execute},
+ * but it is only safe to call {@link #get} if the continuation is done.
*/
public abstract class SpawnContinuation {
public static SpawnContinuation immediate(SpawnResult... spawnResults) {
@@ -43,43 +47,8 @@
return new Finished(ImmutableList.copyOf(spawnResults));
}
- /**
- * Returns a SpawnContinuation implementation that calls {@link SpawnActionContext#beginExecution}
- * on a {@link SpawnActionContext} instance obtained from the {@link ActionExecutionContext}. This
- * continuation does not have a future, and will actually throw {@link IllegalStateException} when
- * {@link #getFuture} is called. The intended pattern of use is:
- *
- * <pre>
- * SpawnContinuation spawnContinuation =
- * SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext);
- * return new CppLinkActionContinuation(actionExecutionContext, spawnContinuation).execute();
- * </pre>
- *
- * <p>This ensures that the action-specific exception handling for {@link #execute} is centralized
- * in one location.
- */
- public static SpawnContinuation ofBeginExecution(
- Spawn spawn, ActionExecutionContext actionExecutionContext) {
- return new SpawnContinuation() {
- @Override
- public ListenableFuture<?> getFuture() {
- // TODO(ulfjack): This is technically a violation of the SpawnContinuation contract, which
- // requires a non-null value when isDone() returns false. We use this to avoid having to
- // duplicate the exception handling wrapping the execute() call. Clients are supposed to
- // immediately call ActionContinuationOrResult.execute(), which immediately calls
- // SpawnContinuation.execute() without checking interface consistency. We should either
- // clarify in SpawnContinuation that this is a legal use, or refactor the code to avoid
- // this, e.g., by extracting the exception handling code in some other way.
- throw new IllegalStateException();
- }
-
- @Override
- public SpawnContinuation execute() throws ExecException, InterruptedException {
- return actionExecutionContext
- .getContext(SpawnActionContext.class)
- .beginExecution(spawn, actionExecutionContext);
- }
- };
+ public static SpawnContinuation failedWithExecException(ExecException e) {
+ return new FailedWithExecException(e);
}
/**
@@ -124,16 +93,35 @@
@Override
public ListenableFuture<?> getFuture() {
- throw new IllegalStateException();
+ return Futures.immediateFuture(null);
}
@Override
public SpawnContinuation execute() {
- throw new IllegalStateException();
+ return this;
}
public List<SpawnResult> get() {
return spawnResults;
}
}
+
+ private static final class FailedWithExecException extends SpawnContinuation {
+ private final ExecException e;
+
+ FailedWithExecException(ExecException e) {
+ this.e = e;
+ }
+
+ @Override
+ public ListenableFuture<?> getFuture() {
+ // This call does not allocate memory because immediateFuture returns a singleton for null.
+ return Futures.immediateFuture(null);
+ }
+
+ @Override
+ public SpawnContinuation execute() throws ExecException {
+ throw e;
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java
index efec9d9..f760837 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java
@@ -76,23 +76,12 @@
FileWriteActionContext context = getStrategy(actionExecutionContext);
SpawnContinuation first =
- new SpawnContinuation() {
- @Override
- public ListenableFuture<?> getFuture() {
- return null;
- }
-
- @Override
- public SpawnContinuation execute() throws ExecException, InterruptedException {
- return context.beginWriteOutputToFile(
- AbstractFileWriteAction.this,
- actionExecutionContext,
- deterministicWriter,
- makeExecutable,
- isRemotable());
- }
- };
-
+ context.beginWriteOutputToFile(
+ AbstractFileWriteAction.this,
+ actionExecutionContext,
+ deterministicWriter,
+ makeExecutable,
+ isRemotable());
return new ActionContinuationOrResult() {
private SpawnContinuation spawnContinuation = first;
@@ -105,11 +94,11 @@
@Override
public ActionContinuationOrResult execute()
throws ActionExecutionException, InterruptedException {
- SpawnContinuation next;
+ SpawnContinuation nextContinuation;
try {
- next = spawnContinuation.execute();
- if (!next.isDone()) {
- spawnContinuation = next;
+ nextContinuation = spawnContinuation.execute();
+ if (!nextContinuation.isDone()) {
+ spawnContinuation = nextContinuation;
return this;
}
} catch (ExecException e) {
@@ -119,9 +108,9 @@
AbstractFileWriteAction.this);
}
afterWrite(actionExecutionContext);
- return ActionContinuationOrResult.of(ActionResult.create(next.get()));
+ return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
}
- }.execute();
+ };
} catch (ExecException e) {
throw e.toActionExecutionException(
"Writing file for rule '" + Label.print(getOwner().getLabel()) + "'",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java
index c48c116..13c94ae 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java
@@ -16,7 +16,6 @@
import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionContext;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
-import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction.DeterministicWriter;
@@ -36,5 +35,5 @@
DeterministicWriter deterministicWriter,
boolean makeExecutable,
boolean isRemotable)
- throws ExecException, InterruptedException;
+ throws InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java
index 46f46cbc..faa379f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java
@@ -17,7 +17,6 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
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.ExecutionStrategy;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction.DeterministicWriter;
@@ -38,8 +37,7 @@
@Override
public SpawnContinuation expandTemplate(
- TemplateExpansionAction action, ActionExecutionContext ctx)
- throws ExecException, InterruptedException {
+ TemplateExpansionAction action, ActionExecutionContext ctx) throws InterruptedException {
try {
final String expandedTemplate = getExpandedTemplateUnsafe(action, ctx.getPathResolver());
DeterministicWriter deterministicWriter =
@@ -53,7 +51,7 @@
.beginWriteOutputToFile(
action, ctx, deterministicWriter, action.makeExecutable(), /*isRemotable=*/ true);
} catch (IOException e) {
- throw new EnvironmentalExecException(e);
+ return SpawnContinuation.failedWithExecException(new EnvironmentalExecException(e));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index 8e022b2..8d932f4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -55,6 +55,7 @@
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.extra.EnvironmentVariable;
@@ -305,13 +306,11 @@
} catch (CommandLineExpansionException e) {
throw new ActionExecutionException(e, this, /*catastrophe=*/ false);
}
- // This construction ensures that beginExecution and execute are called with identical exception
- // handling, pre-processing, and post-processing, at the expense of two throwaway objects.
- SpawnActionContinuation continuation =
- new SpawnActionContinuation(
- actionExecutionContext,
- SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext));
- return continuation.execute();
+ SpawnContinuation spawnContinuation =
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, actionExecutionContext);
+ return new SpawnActionContinuation(actionExecutionContext, spawnContinuation);
}
private ActionExecutionException toActionExecutionException(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
index f5e2b27..d32b911 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java
@@ -130,24 +130,11 @@
@Override
public final ActionContinuationOrResult beginExecution(
- ActionExecutionContext actionExecutionContext)
- throws ActionExecutionException, InterruptedException {
+ ActionExecutionContext actionExecutionContext) throws InterruptedException {
SpawnContinuation first =
- new SpawnContinuation() {
- @Override
- public ListenableFuture<?> getFuture() {
- return null;
- }
-
- @Override
- public SpawnContinuation execute() throws ExecException, InterruptedException {
- TemplateExpansionContext expansionContext =
- actionExecutionContext.getContext(TemplateExpansionContext.class);
- return expansionContext.expandTemplate(
- TemplateExpansionAction.this, actionExecutionContext);
- }
- };
-
+ actionExecutionContext
+ .getContext(TemplateExpansionContext.class)
+ .expandTemplate(TemplateExpansionAction.this, actionExecutionContext);
return new ActionContinuationOrResult() {
private SpawnContinuation spawnContinuation = first;
@@ -160,11 +147,11 @@
@Override
public ActionContinuationOrResult execute()
throws ActionExecutionException, InterruptedException {
- SpawnContinuation next;
+ SpawnContinuation nextContinuation;
try {
- next = spawnContinuation.execute();
- if (!next.isDone()) {
- spawnContinuation = next;
+ nextContinuation = spawnContinuation.execute();
+ if (!nextContinuation.isDone()) {
+ spawnContinuation = nextContinuation;
return this;
}
} catch (ExecException e) {
@@ -173,9 +160,9 @@
actionExecutionContext.getVerboseFailures(),
TemplateExpansionAction.this);
}
- return ActionContinuationOrResult.of(ActionResult.create(next.get()));
+ return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
}
- }.execute();
+ };
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionContext.java
index 48e26cb..00c18ab 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionContext.java
@@ -16,11 +16,10 @@
import com.google.devtools.build.lib.actions.ActionContext;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
-import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.SpawnContinuation;
/** The action context for {@link TemplateExpansionAction} instances */
public interface TemplateExpansionContext extends ActionContext {
SpawnContinuation expandTemplate(TemplateExpansionAction action, ActionExecutionContext ctx)
- throws ExecException, InterruptedException;
+ throws InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
index 9ef921b..d46462d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis.test;
+import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.actions.ActionContext;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -102,12 +103,12 @@
@Override
public ListenableFuture<?> getFuture() {
- throw new IllegalStateException();
+ return Futures.immediateFuture(null);
}
@Override
public TestAttemptContinuation execute() {
- throw new IllegalStateException();
+ return this;
}
@Override
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();
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java
index eb7d3a1..17ab7bc 100644
--- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java
+++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java
@@ -491,23 +491,17 @@
// parent context as a lock to make it thread-safe (see dump() below).
FileOutErr originalOutErr = actionExecutionContext.getFileOutErr();
FileOutErr grepOutErr = originalOutErr.childOutErr();
- SettableFuture<InputStream> future = SettableFuture.create();
ActionExecutionContext grepContext = actionExecutionContext.withFileOutErr(grepOutErr);
+ SpawnContinuation spawnContinuation;
try {
- process(
- executor,
- future,
- SpawnContinuation.ofBeginExecution(spawn, grepContext).execute(),
- output,
- grepContext,
- actionExecutionContext);
- } catch (ExecException e) {
- dump(grepContext, actionExecutionContext);
- future.setException(e);
+ spawnContinuation =
+ grepContext.getContext(SpawnActionContext.class).beginExecution(spawn, grepContext);
} catch (InterruptedException e) {
dump(grepContext, actionExecutionContext);
- future.cancel(false);
+ return Futures.immediateCancelledFuture();
}
+ SettableFuture<InputStream> future = SettableFuture.create();
+ process(executor, future, spawnContinuation, output, grepContext, actionExecutionContext);
return future;
}
@@ -518,22 +512,25 @@
ActionInput output,
ActionExecutionContext actionExecutionContext,
ActionExecutionContext originalActionExecutionContext) {
- if (continuation.isDone()) {
- List<SpawnResult> results = continuation.get();
- dump(actionExecutionContext, originalActionExecutionContext);
- SpawnResult result = Iterables.getLast(results);
- InputStream stream = result.getInMemoryOutput(output);
- try {
- future.set(
- stream == null ? actionExecutionContext.getInputPath(output).getInputStream() : stream);
- } catch (IOException e) {
- future.setException(e);
- }
- } else {
- continuation
- .getFuture()
- .addListener(
- () -> {
+ continuation
+ .getFuture()
+ .addListener(
+ () -> {
+ if (continuation.isDone()) {
+ List<SpawnResult> results = continuation.get();
+ dump(actionExecutionContext, originalActionExecutionContext);
+ SpawnResult result = Iterables.getLast(results);
+ InputStream stream = result.getInMemoryOutput(output);
+ try {
+ InputStream finalResult =
+ stream == null
+ ? actionExecutionContext.getInputPath(output).getInputStream()
+ : stream;
+ future.set(finalResult);
+ } catch (IOException e) {
+ future.setException(e);
+ }
+ } else {
try {
SpawnContinuation next = continuation.execute();
process(
@@ -550,9 +547,9 @@
dump(actionExecutionContext, originalActionExecutionContext);
future.cancel(false);
}
- },
- executor);
- }
+ }
+ },
+ executor);
}
private static void dump(ActionExecutionContext fromContext, ActionExecutionContext toContext) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index ec01eaa..4c16242 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -48,6 +48,7 @@
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.extra.CppCompileInfo;
@@ -1259,13 +1260,16 @@
clearAdditionalInputs();
}
+ SpawnContinuation spawnContinuation =
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, spawnContext);
return new CppCompileActionContinuation(
- actionExecutionContext,
- spawnContext,
- showIncludesFilterForStdout,
- showIncludesFilterForStderr,
- SpawnContinuation.ofBeginExecution(spawn, spawnContext))
- .execute();
+ actionExecutionContext,
+ spawnContext,
+ showIncludesFilterForStdout,
+ showIncludesFilterForStderr,
+ spawnContinuation);
}
protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index 8722a51..73871d0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -42,6 +42,7 @@
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.extra.CppLinkInfo;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -293,8 +294,10 @@
}
Spawn spawn = createSpawn(actionExecutionContext);
SpawnContinuation spawnContinuation =
- SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext);
- return new CppLinkActionContinuation(actionExecutionContext, spawnContinuation).execute();
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, actionExecutionContext);
+ return new CppLinkActionContinuation(actionExecutionContext, spawnContinuation);
}
private Spawn createSpawn(ActionExecutionContext actionExecutionContext)
@@ -544,10 +547,10 @@
throws ActionExecutionException, InterruptedException {
try {
SpawnContinuation nextContinuation = spawnContinuation.execute();
- if (nextContinuation.isDone()) {
- return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
+ if (!nextContinuation.isDone()) {
+ return new CppLinkActionContinuation(actionExecutionContext, nextContinuation);
}
- return new CppLinkActionContinuation(actionExecutionContext, nextContinuation);
+ return ActionContinuationOrResult.of(ActionResult.create(nextContinuation.get()));
} catch (ExecException e) {
throw e.toActionExecutionException(
"Linking of rule '" + getOwner().getLabel() + "'",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
index 4df07ec..2d64663 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
@@ -48,6 +48,7 @@
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -327,12 +328,11 @@
} catch (CommandLineExpansionException e) {
throw new ActionExecutionException(e, this, /*catastrophe=*/ false);
}
- JavaActionContinuation continuation =
- new JavaActionContinuation(
- actionExecutionContext,
- reducedClasspath,
- SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext));
- return continuation.execute();
+ SpawnContinuation spawnContinuation =
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, actionExecutionContext);
+ return new JavaActionContinuation(actionExecutionContext, reducedClasspath, spawnContinuation);
}
@Override
@@ -591,11 +591,12 @@
} catch (CommandLineExpansionException e) {
throw new ActionExecutionException(e, JavaCompileAction.this, /*catastrophe=*/ false);
}
+ SpawnContinuation fallbackContinuation =
+ actionExecutionContext
+ .getContext(SpawnActionContext.class)
+ .beginExecution(spawn, actionExecutionContext);
return new JavaFallbackActionContinuation(
- actionExecutionContext,
- results,
- SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext))
- .execute();
+ actionExecutionContext, results, fallbackContinuation);
} catch (IOException e) {
throw toActionExecutionException(
new EnvironmentalExecException(e), actionExecutionContext.getVerboseFailures());
diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
index 88e5b67..2c8b02d 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
@@ -257,7 +257,9 @@
.setRunnerName("test")
.build();
when(spawnActionContext.beginExecution(any(), any()))
- .thenThrow(new SpawnExecException("test failed", failSpawnResult, false))
+ .thenReturn(
+ SpawnContinuation.failedWithExecException(
+ new SpawnExecException("test failed", failSpawnResult, false)))
.thenReturn(SpawnContinuation.immediate(passSpawnResult));
ActionExecutionContext actionExecutionContext =
@@ -454,11 +456,12 @@
stream.write((TestLogHelper.HEADER_DELIMITER + "\n").getBytes(UTF_8));
stream.write("This will appear in the test output: foo\n".getBytes(UTF_8));
}
- throw new SpawnExecException(
- "Failure!!",
- expectedSpawnResult,
- /*forciblyRunRemotely=*/ false,
- /*catastrophe=*/ false);
+ return SpawnContinuation.failedWithExecException(
+ new SpawnExecException(
+ "Failure!!",
+ expectedSpawnResult,
+ /*forciblyRunRemotely=*/ false,
+ /*catastrophe=*/ false));
} else {
return SpawnContinuation.immediate(
new SpawnResult.Builder()
@@ -554,11 +557,12 @@
stream.write((TestLogHelper.HEADER_DELIMITER + "\n").getBytes(UTF_8));
stream.write("This will appear in the test output: foo\n".getBytes(UTF_8));
}
- throw new SpawnExecException(
- "Failure!!",
- testSpawnResult,
- /*forciblyRunRemotely=*/ false,
- /*catastrophe=*/ false);
+ return SpawnContinuation.failedWithExecException(
+ new SpawnExecException(
+ "Failure!!",
+ testSpawnResult,
+ /*forciblyRunRemotely=*/ false,
+ /*catastrophe=*/ false));
} else {
String testName =
OS.getCurrent() == OS.WINDOWS
