Skip to content

Commit b7c1ad2

Browse files
larsrc-googlelarsrc-google
committed
Fix rare crash in dynamic execution where both branches got cancelled.
Races in `stopBranch` caused the losing branch to throw `DynamicInterruptedException`, which could then get to that branch's listener before the winning branch actually cancelled. RELNOTES: None. PiperOrigin-RevId: 383126912
1 parent 1962a59 commit b7c1ad2

File tree

4 files changed

+56
-20
lines changed

4 files changed

+56
-20
lines changed

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

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,10 +268,7 @@ private static ImmutableList<SpawnResult> waitBranches(
268268
ImmutableList<SpawnResult> remoteResult = waitBranch(remoteBranch);
269269

270270
if (remoteResult != null && localResult != null) {
271-
throw new AssertionError(
272-
String.format(
273-
"Neither branch of %s cancelled the other one.",
274-
spawn.getResourceOwner().getPrimaryOutput().prettyPrint()));
271+
throw new AssertionError("Neither branch cancelled the other one.");
275272
} else if (localResult != null) {
276273
return localResult;
277274
} else if (remoteResult != null) {
@@ -507,6 +504,23 @@ ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
507504
DynamicSpawnStrategy.this.options,
508505
actionExecutionContext,
509506
spawn));
507+
} catch (DynamicInterruptedException e) {
508+
// This exception can be thrown due to races in stopBranch(), in which case
509+
// the branch that lost the race may not have been cancelled yet. Cancel it here
510+
// to prevent the listener from cross-cancelling.
511+
localBranch.cancel(true);
512+
throw e;
513+
} catch (
514+
@SuppressWarnings("InterruptedExceptionSwallowed")
515+
Throwable e) {
516+
if (options.debugSpawnScheduler) {
517+
logger.atInfo().log(
518+
"Local branch of %s failed with %s: '%s'",
519+
spawn.getResourceOwner(),
520+
e.getClass().getSimpleName(),
521+
e.getMessage());
522+
}
523+
throw e;
510524
} finally {
511525
localDone.release();
512526
}
@@ -554,6 +568,23 @@ public ImmutableList<SpawnResult> callImpl(ActionExecutionContext context)
554568
spawn));
555569
delayLocalExecution.set(true);
556570
return spawnResults;
571+
} catch (DynamicInterruptedException e) {
572+
// This exception can be thrown due to races in stopBranch(), in which case
573+
// the branch that lost the race may not have been cancelled yet. Cancel it here
574+
// to prevent the listener from cross-cancelling.
575+
remoteBranch.cancel(true);
576+
throw e;
577+
} catch (
578+
@SuppressWarnings("InterruptedExceptionSwallowed")
579+
Throwable e) {
580+
if (options.debugSpawnScheduler) {
581+
logger.atInfo().log(
582+
"Remote branch of %s failed with %s: '%s'",
583+
spawn.getResourceOwner(),
584+
e.getClass().getSimpleName(),
585+
e.getMessage());
586+
}
587+
throw e;
557588
} finally {
558589
remoteDone.release();
559590
}

‎src/test/java/com/google/devtools/build/lib/dynamic/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,11 @@ java_test(
1818
srcs = ["DynamicSpawnStrategyUnitTest.java"],
1919
deps = [
2020
"//src/main/java/com/google/devtools/build/lib/actions",
21+
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
2122
"//src/main/java/com/google/devtools/build/lib/dynamic",
2223
"//src/main/java/com/google/devtools/build/lib/exec:execution_policy",
24+
"//src/main/java/com/google/devtools/build/lib/vfs",
25+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
2326
"//src/main/protobuf:failure_details_java_proto",
2427
"//src/test/java/com/google/devtools/build/lib/exec/util",
2528
"//src/test/java/com/google/devtools/build/lib/testutil",

‎src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import static com.google.common.base.Preconditions.checkArgument;
1717
import static com.google.common.base.Preconditions.checkState;
1818
import static com.google.common.truth.Truth.assertThat;
19+
import static com.google.common.truth.TruthJUnit.assume;
1920
import static junit.framework.TestCase.fail;
2021
import static org.junit.Assert.assertThrows;
2122

@@ -571,6 +572,7 @@ public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalFailsLater() throw
571572

572573
@Test
573574
public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() throws Exception {
575+
assume().that(legacyBehavior).isFalse();
574576
MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy");
575577

576578
MockSpawnStrategy remoteStrategy =
@@ -592,6 +594,7 @@ public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() thro
592594

593595
@Test
594596
public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalRunsNothing() throws Exception {
597+
assume().that(legacyBehavior).isFalse();
595598
MockSpawnStrategy localStrategy =
596599
new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false);
597600

@@ -723,6 +726,7 @@ public void actionFailsIfLocalAndRemoteFail() throws Exception {
723726

724727
@Test
725728
public void actionFailsIfLocalAndRemoteRunNothing() throws Exception {
729+
assume().that(legacyBehavior).isFalse();
726730
MockSpawnStrategy localStrategy =
727731
new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false);
728732

‎src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@
2828
import static org.mockito.Mockito.when;
2929

3030
import com.google.common.collect.ImmutableList;
31+
import com.google.common.util.concurrent.Futures;
3132
import com.google.devtools.build.lib.actions.ActionExecutionContext;
3233
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
34+
import com.google.devtools.build.lib.actions.ArtifactRoot;
3335
import com.google.devtools.build.lib.actions.DynamicStrategyRegistry;
3436
import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy;
3537
import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy.StopConcurrentSpawns;
@@ -41,8 +43,12 @@
4143
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
4244
import com.google.devtools.build.lib.server.FailureDetails.Execution;
4345
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
46+
import com.google.devtools.build.lib.testutil.Scratch;
4447
import com.google.devtools.build.lib.testutil.TestFileOutErr;
4548
import com.google.devtools.build.lib.testutil.TestUtils;
49+
import com.google.devtools.build.lib.vfs.Path;
50+
import com.google.devtools.build.lib.vfs.PathFragment;
51+
import java.io.IOException;
4652
import java.util.Optional;
4753
import java.util.concurrent.ExecutorService;
4854
import java.util.concurrent.Executors;
@@ -85,8 +91,15 @@ private static void verifyNoInteractions(Object... objs) {
8591

8692
@Mock private Function<Spawn, Optional<Spawn>> mockGetPostProcessingSpawn;
8793

94+
private Scratch scratch;
95+
private Path execDir;
96+
private ArtifactRoot rootDir;
97+
8898
@Before
89-
public void initMocks() {
99+
public void initMocks() throws IOException {
100+
scratch = new Scratch();
101+
execDir = scratch.dir("/base/exec");
102+
rootDir = ArtifactRoot.asDerivedRoot(execDir, "root");
90103
MockitoAnnotations.initMocks(this);
91104
// Mockito can't see that we want the function to return Optional.empty() instead
92105
// of null on apply by default (thanks generic type erasure). Set that up ourselves.
@@ -270,21 +283,6 @@ public void exec_runAnywhereSpawn_runsLocalPostProcessingSpawn() throws Exceptio
270283
assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT);
271284
}
272285

273-
@Test
274-
public void waitBranches_givesDebugOutputOnWeirdCases() throws Exception {
275-
Spawn spawn =
276-
new SpawnBuilder()
277-
.withOwnerPrimaryOutput(new SourceArtifact(rootDir, PathFragment.create("/foo"), null))
278-
.build();
279-
AssertionError error =
280-
assertThrows(
281-
AssertionError.class,
282-
() ->
283-
DynamicSpawnStrategy.waitBranches(
284-
Futures.immediateFuture(null), Futures.immediateFuture(null), spawn));
285-
assertThat(error).hasMessageThat().contains("Neither branch of /foo completed.");
286-
}
287-
288286
@Test
289287
public void exec_runAnywhereSpawn_localCantExec_runsRemote() throws Exception {
290288
Spawn spawn = new SpawnBuilder().build();

0 commit comments

Comments
 (0)