Get the mode from the Branch object instead of passing it separately when possible.
This doesn't change any functionality.
Also a bit of comment cleanup.
PiperOrigin-RevId: 421241010
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/Branch.java b/src/main/java/com/google/devtools/build/lib/dynamic/Branch.java
index 951d0fb..6cb515b 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/Branch.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/Branch.java
@@ -55,29 +55,32 @@
*/
protected final SettableFuture<ImmutableList<SpawnResult>> future = SettableFuture.create();
+ /**
+ * The strategy (local or remote) that cancelled the other one. Null until one has been cancelled.
+ * This object is shared between the local and remote branch of an action.
+ */
protected final AtomicReference<DynamicMode> strategyThatCancelled;
- /** Semaphore that indicates whether this branch is done, i.e. either completed or cancelled. */
+ /**
+ * Semaphore that indicates whether this branch is done, i.e. either completed or cancelled. This
+ * is needed to wait for the branch to finish its own cleanup (e.g. terminating subprocesses) once
+ * it has been cancelled.
+ */
protected final Semaphore done = new Semaphore(0);
protected final DynamicExecutionOptions options;
- private final DynamicMode mode;
protected final ActionExecutionContext context;
/**
* Creates a new branch of dynamic execution.
*
- * @param mode the dynamic mode that this branch represents (e.g. {@link DynamicMode#REMOTE}).
- * Used to qualify temporary files.
* @param context the action execution context given to the dynamic strategy, used to obtain the
* final location of the stdout/stderr
*/
Branch(
- DynamicMode mode,
ActionExecutionContext context,
Spawn spawn,
AtomicReference<DynamicMode> strategyThatCancelled,
DynamicExecutionOptions options) {
- this.mode = mode;
this.context = context;
this.spawn = spawn;
this.strategyThatCancelled = strategyThatCancelled;
@@ -112,6 +115,8 @@
return spawn;
}
+ public abstract DynamicMode getMode();
+
/** Returns a human-readable description of what we can tell about the state of this Future. */
String branchState() {
return (isCancelled() ? "cancelled" : "not cancelled")
@@ -169,7 +174,7 @@
*/
@Override
public final ImmutableList<SpawnResult> call() throws InterruptedException, ExecException {
- FileOutErr fileOutErr = getSuffixedFileOutErr(context.getFileOutErr(), "." + mode.name());
+ FileOutErr fileOutErr = getSuffixedFileOutErr(context.getFileOutErr(), "." + getMode().name());
ImmutableList<SpawnResult> results = null;
ExecException exception = null;
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
index d73b607..3eeccd9 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java
@@ -188,8 +188,7 @@
return false;
}
List<SandboxedSpawnStrategy> remoteStrategies =
- dynamicStrategyRegistry.getDynamicSpawnActionContexts(
- spawn, DynamicStrategyRegistry.DynamicMode.REMOTE);
+ dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, REMOTE);
return remoteStrategies.stream().anyMatch(s -> s.canExec(spawn, actionContextRegistry));
}
@@ -221,9 +220,6 @@
debugLog("Dynamic execution of %s beginning%n", spawn.getResourceOwner().prettyPrint());
// else both can exec. Fallthrough to below.
- // Semaphores to track termination of each branch. These are necessary to wait for the branch
- // to finish its own cleanup (e.g. terminating subprocesses) once it has been cancelled.
-
AtomicReference<DynamicMode> strategyThatCancelled = new AtomicReference<>(null);
LocalBranch localBranch =
@@ -379,13 +375,13 @@
+ "remote strategies are %s.%n",
spawn.getResourceOwner().prettyPrint(),
executionPolicy.canRunRemotely() ? "allows" : "forbids",
- dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, DynamicMode.REMOTE));
+ dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, REMOTE));
debugLog(
"Dynamic execution of %s can only be done locally: Remote execution policy %s it, "
+ "remote strategies are %s.%n",
spawn.getResourceOwner().prettyPrint(),
executionPolicy.canRunRemotely() ? "allows" : "forbids",
- dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, DynamicMode.REMOTE));
+ dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, REMOTE));
return LocalBranch.runLocally(
spawn, actionExecutionContext, null, getExtraSpawnForLocalExecution);
} else if (options.skipFirstBuild && firstBuild) {
@@ -467,7 +463,7 @@
throws ExecException, InterruptedException {
ImmutableList<SpawnResult> localResult;
try {
- localResult = waitBranch(localBranch, options, LOCAL, context);
+ localResult = waitBranch(localBranch, options, context);
} catch (ExecException | InterruptedException | RuntimeException e) {
if (options.debugSpawnScheduler) {
context
@@ -482,7 +478,7 @@
throw e;
}
- ImmutableList<SpawnResult> remoteResult = waitBranch(remoteBranch, options, REMOTE, context);
+ ImmutableList<SpawnResult> remoteResult = waitBranch(remoteBranch, options, context);
if (remoteResult != null && localResult != null) {
throw new AssertionError(
@@ -520,11 +516,9 @@
*/
@Nullable
private static ImmutableList<SpawnResult> waitBranch(
- Branch branch,
- DynamicExecutionOptions options,
- DynamicMode mode,
- ActionExecutionContext context)
+ Branch branch, DynamicExecutionOptions options, ActionExecutionContext context)
throws ExecException, InterruptedException {
+ DynamicMode mode = branch.getMode();
try {
ImmutableList<SpawnResult> spawnResults = branch.getResults();
if (spawnResults == null && options.debugSpawnScheduler) {
@@ -594,9 +588,6 @@
*
* @param otherBranch The other branch, the one that should be cancelled.
* @param cancellingBranch The branch that is performing the cancellation.
- * @param cancellingStrategy identifier of the strategy that is performing the cancellation. Used
- * to prevent cross-cancellations and to check that the same strategy doesn't issue the
- * cancellation twice.
* @param strategyThatCancelled name of the first strategy that executed this method, or a null
* reference if this is the first time this method is called. If not null, we expect the value
* referenced by this to be different than {@code cancellingStrategy}, or else we have a bug.
@@ -609,11 +600,11 @@
static void stopBranch(
Branch otherBranch,
Branch cancellingBranch,
- DynamicMode cancellingStrategy,
AtomicReference<DynamicMode> strategyThatCancelled,
DynamicExecutionOptions options,
ActionExecutionContext context)
throws InterruptedException {
+ DynamicMode cancellingStrategy = cancellingBranch.getMode();
if (cancellingBranch.isCancelled()) {
// TODO(b/173020239): Determine why stopBranch() can be called when cancellingBranch is
// cancelled.
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/LocalBranch.java b/src/main/java/com/google/devtools/build/lib/dynamic/LocalBranch.java
index 59f4a7f..90ff49b 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/LocalBranch.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/LocalBranch.java
@@ -61,12 +61,17 @@
IgnoreFailureCheck ignoreFailureCheck,
Function<Spawn, Optional<Spawn>> getExtraSpawnForLocalExecution,
AtomicBoolean delayLocalExecution) {
- super(DynamicMode.LOCAL, actionExecutionContext, spawn, strategyThatCancelled, options);
+ super(actionExecutionContext, spawn, strategyThatCancelled, options);
this.ignoreFailureCheck = ignoreFailureCheck;
this.getExtraSpawnForLocalExecution = getExtraSpawnForLocalExecution;
this.delayLocalExecution = delayLocalExecution;
}
+ @Override
+ public DynamicMode getMode() {
+ return LOCAL;
+ }
+
/**
* Try to run the given spawn locally.
*
@@ -164,7 +169,7 @@
(exitCode, errorMessage, outErr) -> {
maybeIgnoreFailure(exitCode, errorMessage, outErr);
DynamicSpawnStrategy.stopBranch(
- remoteBranch, this, LOCAL, strategyThatCancelled, options, this.context);
+ remoteBranch, this, strategyThatCancelled, options, this.context);
},
getExtraSpawnForLocalExecution);
} catch (DynamicInterruptedException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/dynamic/RemoteBranch.java b/src/main/java/com/google/devtools/build/lib/dynamic/RemoteBranch.java
index 20d55a5..02b28dc 100644
--- a/src/main/java/com/google/devtools/build/lib/dynamic/RemoteBranch.java
+++ b/src/main/java/com/google/devtools/build/lib/dynamic/RemoteBranch.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.dynamic;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.devtools.build.lib.actions.DynamicStrategyRegistry.DynamicMode.REMOTE;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
@@ -56,11 +57,16 @@
DynamicExecutionOptions options,
IgnoreFailureCheck ignoreFailureCheck,
AtomicBoolean delayLocalExecution) {
- super(DynamicMode.REMOTE, actionExecutionContext, spawn, strategyThatCancelled, options);
+ super(actionExecutionContext, spawn, strategyThatCancelled, options);
this.ignoreFailureCheck = ignoreFailureCheck;
this.delayLocalExecution = delayLocalExecution;
}
+ @Override
+ public DynamicMode getMode() {
+ return REMOTE;
+ }
+
/**
* Try to run the given spawn remotely. If successful, updates {@link #delayLocalExecution} if
* there was a cache hit among the results.
@@ -78,7 +84,7 @@
actionExecutionContext.getContext(DynamicStrategyRegistry.class);
for (SandboxedSpawnStrategy strategy :
- dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, DynamicMode.REMOTE)) {
+ dynamicStrategyRegistry.getDynamicSpawnActionContexts(spawn, REMOTE)) {
if (strategy.canExec(spawn, actionExecutionContext)) {
ImmutableList<SpawnResult> results =
strategy.exec(spawn, actionExecutionContext, stopConcurrentSpawns);
@@ -141,12 +147,7 @@
(exitCode, errorMessage, outErr) -> {
maybeIgnoreFailure(exitCode, errorMessage, outErr);
DynamicSpawnStrategy.stopBranch(
- localBranch,
- this,
- DynamicMode.REMOTE,
- strategyThatCancelled,
- options,
- this.context);
+ localBranch, this, strategyThatCancelled, options, this.context);
},
delayLocalExecution);
} catch (DynamicInterruptedException e) {
