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) {