Skip to content

Commit 5966762

Browse files
fmeumfmeum
committed
Fix
1 parent 60c62ee commit 5966762

File tree

4 files changed

+51
-67
lines changed

4 files changed

+51
-67
lines changed

‎src/main/java/com/google/devtools/build/lib/dynamic/Branch.java‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.google.common.collect.ImmutableList;
2020
import com.google.common.flogger.GoogleLogger;
2121
import com.google.common.util.concurrent.ListeningExecutorService;
22+
import com.google.common.util.concurrent.MoreExecutors;
2223
import com.google.common.util.concurrent.SettableFuture;
2324
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2425
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry.DynamicMode;
@@ -48,8 +49,10 @@ abstract class Branch implements Callable<ImmutableList<SpawnResult>> {
4849
* #callImpl(ActionExecutionContext)} yet.
4950
*/
5051
protected final AtomicBoolean starting = new AtomicBoolean(true);
52+
5153
/** The {@link Spawn} this branch is running. */
5254
protected final Spawn spawn;
55+
5356
/**
5457
* The {@link SettableFuture} with the results from running the spawn. Must not be null if
5558
* execution succeeded.
@@ -61,6 +64,7 @@ abstract class Branch implements Callable<ImmutableList<SpawnResult>> {
6164
* This object is shared between the local and remote branch of an action.
6265
*/
6366
protected final AtomicReference<DynamicMode> strategyThatCancelled;
67+
6468
/**
6569
* Semaphore that indicates whether this branch is done, i.e. either completed or cancelled. This
6670
* is needed to wait for the branch to finish its own cleanup (e.g. terminating subprocesses) once
@@ -71,6 +75,8 @@ abstract class Branch implements Callable<ImmutableList<SpawnResult>> {
7175
protected final DynamicExecutionOptions options;
7276
protected final ActionExecutionContext context;
7377

78+
protected Branch otherBranch;
79+
7480
/**
7581
* Creates a new branch of dynamic execution.
7682
*
@@ -130,6 +136,41 @@ public void execute(ListeningExecutorService executor) {
130136
future.setFuture(executor.submit(this));
131137
}
132138

139+
/** Sets up the {@link Future} used in the current branch to know what other branch to cancel. */
140+
protected void prepareFuture(Branch otherBranch) {
141+
this.otherBranch = otherBranch;
142+
future.addListener(
143+
() -> {
144+
if (starting.compareAndSet(true, false)) {
145+
// If the current branch got cancelled before even starting, we release its semaphore
146+
// for it.
147+
done.release();
148+
}
149+
// If the current branch succeeds, there is no need to keep the other branch running.
150+
// If the current branch fails, cancel the other branch as well. However, that one may
151+
// in turn cancel us, thus causing an interruption. Don't consider that a failure as
152+
// we otherwise risk canceling both branches.
153+
var state = future.state();
154+
if (state == Future.State.SUCCESS
155+
|| (state == Future.State.FAILED
156+
&& !(future.exceptionNow() instanceof InterruptedException))) {
157+
otherBranch.cancel();
158+
}
159+
if (options.debugSpawnScheduler) {
160+
logger.atInfo().log(
161+
"In listener callback, the future of the remote branch is %s",
162+
future.state().name());
163+
try {
164+
future.get();
165+
} catch (InterruptedException | ExecutionException e) {
166+
logger.atInfo().withCause(e).log(
167+
"The future of the remote branch failed with an exception.");
168+
}
169+
}
170+
},
171+
MoreExecutors.directExecutor());
172+
}
173+
133174
/**
134175
* Moves a set of stdout/stderr files over another one. Errors during the move are logged and
135176
* swallowed.

‎src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
import java.util.concurrent.CancellationException;
5555
import java.util.concurrent.ExecutionException;
5656
import java.util.concurrent.ExecutorService;
57+
import java.util.concurrent.Future.State;
5758
import java.util.concurrent.atomic.AtomicBoolean;
5859
import java.util.concurrent.atomic.AtomicReference;
5960
import java.util.function.Function;
@@ -708,7 +709,7 @@ static void stopBranch(
708709
// This can happen if the other branch is local under local_lockfree and has returned
709710
// its result but not yet cancelled this branch, or if the other branch was already
710711
// cancelled for other reasons. In the latter case, we are good to continue.
711-
if (!otherBranch.isCancelled()) {
712+
if (otherBranch.future.state() == State.SUCCESS) {
712713
throw new DynamicInterruptedException(
713714
String.format(
714715
"Execution of %s strategy stopped because %s strategy could not be cancelled",

‎src/main/java/com/google/devtools/build/lib/dynamic/LocalBranch.java‎

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.google.common.annotations.VisibleForTesting;
2121
import com.google.common.collect.ImmutableList;
2222
import com.google.common.flogger.GoogleLogger;
23-
import com.google.common.util.concurrent.MoreExecutors;
2423
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2524
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry;
2625
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry.DynamicMode;
@@ -29,14 +28,14 @@
2928
import com.google.devtools.build.lib.actions.Spawn;
3029
import com.google.devtools.build.lib.actions.SpawnResult;
3130
import com.google.devtools.build.lib.actions.SpawnResult.Status;
31+
import com.google.devtools.build.lib.actions.SpawnStrategy;
3232
import com.google.devtools.build.lib.dynamic.DynamicExecutionModule.IgnoreFailureCheck;
3333
import com.google.devtools.build.lib.profiler.Profiler;
3434
import com.google.devtools.build.lib.profiler.SilentCloseable;
3535
import com.google.devtools.build.lib.util.io.FileOutErr;
3636
import java.time.Duration;
3737
import java.time.Instant;
3838
import java.util.Optional;
39-
import java.util.concurrent.ExecutionException;
4039
import java.util.concurrent.atomic.AtomicBoolean;
4140
import java.util.concurrent.atomic.AtomicReference;
4241
import java.util.function.Function;
@@ -50,7 +49,6 @@
5049
class LocalBranch extends Branch {
5150
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
5251

53-
private RemoteBranch remoteBranch;
5452
private final IgnoreFailureCheck ignoreFailureCheck;
5553
private final Function<Spawn, Optional<Spawn>> getExtraSpawnForLocalExecution;
5654
private final AtomicBoolean delayLocalExecution;
@@ -138,38 +136,12 @@ private static ImmutableList<SpawnResult> runSpawnLocally(
138136
throw new AssertionError("canExec passed but no usable local strategy for action " + spawn);
139137
}
140138

141-
/** Sets up the {@link Future} used in the local branch to know what remote branch to cancel. */
142-
protected void prepareFuture(RemoteBranch remoteBranch) {
143-
// TODO(b/203094728): Maybe generify this method and move it up.
144-
this.remoteBranch = remoteBranch;
145-
future.addListener(
146-
() -> {
147-
if (starting.compareAndSet(true, false)) {
148-
// If the local branch got cancelled before even starting, we release its semaphore
149-
// for it.
150-
done.release();
151-
}
152-
if (!future.isCancelled()) {
153-
remoteBranch.cancel();
154-
}
155-
if (options.debugSpawnScheduler) {
156-
logger.atInfo().log(
157-
"In listener callback, the future of the local branch is %s",
158-
future.state().name());
159-
try {
160-
future.get();
161-
} catch (InterruptedException | ExecutionException e) {
162-
logger.atInfo().withCause(e).log(
163-
"The future of the local branch failed with an exception.");
164-
}
165-
}
166-
},
167-
MoreExecutors.directExecutor());
168-
}
169-
170139
@Override
171140
ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
172141
throws InterruptedException, ExecException {
142+
if (otherBranch == null) {
143+
throw new IllegalStateException("Initialize not called");
144+
}
173145
try {
174146
if (!starting.compareAndSet(true, false)) {
175147
// If we ever get here, it's because we were cancelled early and the listener
@@ -190,7 +162,7 @@ ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
190162
maybeIgnoreFailure(exitCode, errorMessage, outErr);
191163
}
192164
DynamicSpawnStrategy.stopBranch(
193-
remoteBranch, this, strategyThatCancelled, options, this.context);
165+
otherBranch, this, strategyThatCancelled, options, this.context);
194166
},
195167
getExtraSpawnForLocalExecution);
196168
} catch (DynamicInterruptedException e) {

‎src/main/java/com/google/devtools/build/lib/dynamic/RemoteBranch.java‎

Lines changed: 3 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.google.common.annotations.VisibleForTesting;
2121
import com.google.common.collect.ImmutableList;
2222
import com.google.common.flogger.GoogleLogger;
23-
import com.google.common.util.concurrent.MoreExecutors;
2423
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2524
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry;
2625
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry.DynamicMode;
@@ -29,10 +28,10 @@
2928
import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy.StopConcurrentSpawns;
3029
import com.google.devtools.build.lib.actions.Spawn;
3130
import com.google.devtools.build.lib.actions.SpawnResult;
31+
import com.google.devtools.build.lib.actions.SpawnStrategy;
3232
import com.google.devtools.build.lib.dynamic.DynamicExecutionModule.IgnoreFailureCheck;
3333
import com.google.devtools.build.lib.events.Event;
3434
import com.google.devtools.build.lib.util.io.FileOutErr;
35-
import java.util.concurrent.ExecutionException;
3635
import java.util.concurrent.atomic.AtomicBoolean;
3736
import java.util.concurrent.atomic.AtomicReference;
3837
import javax.annotation.Nullable;
@@ -45,7 +44,6 @@
4544
class RemoteBranch extends Branch {
4645
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
4746

48-
private LocalBranch localBranch;
4947
private final IgnoreFailureCheck ignoreFailureCheck;
5048
private final AtomicBoolean delayLocalExecution;
5149

@@ -109,38 +107,10 @@ static ImmutableList<SpawnResult> runRemotely(
109107
throw new AssertionError("canExec passed but no usable remote strategy for action " + spawn);
110108
}
111109

112-
/** Sets up the future for this branch, once the other branch is available. */
113-
public void prepareFuture(LocalBranch localBranch) {
114-
this.localBranch = localBranch;
115-
future.addListener(
116-
() -> {
117-
if (starting.compareAndSet(true, false)) {
118-
// If the remote branch got cancelled before even starting, we release its semaphore
119-
// for it.
120-
done.release();
121-
}
122-
if (!future.isCancelled()) {
123-
localBranch.cancel();
124-
}
125-
if (options.debugSpawnScheduler) {
126-
logger.atInfo().log(
127-
"In listener callback, the future of the remote branch is %s",
128-
future.state().name());
129-
try {
130-
future.get();
131-
} catch (InterruptedException | ExecutionException e) {
132-
logger.atInfo().withCause(e).log(
133-
"The future of the remote branch failed with an exception.");
134-
}
135-
}
136-
},
137-
MoreExecutors.directExecutor());
138-
}
139-
140110
@Override
141111
public ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
142112
throws InterruptedException, ExecException {
143-
if (localBranch == null) {
113+
if (otherBranch == null) {
144114
throw new IllegalStateException("Initialize not called");
145115
}
146116
try {
@@ -158,7 +128,7 @@ public ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
158128
maybeIgnoreFailure(exitCode, errorMessage, outErr);
159129
}
160130
DynamicSpawnStrategy.stopBranch(
161-
localBranch, this, strategyThatCancelled, options, this.context);
131+
otherBranch, this, strategyThatCancelled, options, this.context);
162132
},
163133
delayLocalExecution);
164134
} catch (DynamicInterruptedException e) {

0 commit comments

Comments
 (0)