From b7c1ad2aff91105659299723a712b72eea943040 Mon Sep 17 00:00:00 2001 From: larsrc Date: Mon, 5 Jul 2021 06:29:38 -0700 Subject: [PATCH] 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 --- .../lib/dynamic/DynamicSpawnStrategy.java | 39 +++++++++++++++++-- .../google/devtools/build/lib/dynamic/BUILD | 3 ++ .../lib/dynamic/DynamicSpawnStrategyTest.java | 4 ++ .../dynamic/DynamicSpawnStrategyUnitTest.java | 30 +++++++------- 4 files changed, 56 insertions(+), 20 deletions(-) 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 db769266c5232b..c141b20760150e 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 @@ -268,10 +268,7 @@ private static ImmutableList waitBranches( ImmutableList remoteResult = waitBranch(remoteBranch); if (remoteResult != null && localResult != null) { - throw new AssertionError( - String.format( - "Neither branch of %s cancelled the other one.", - spawn.getResourceOwner().getPrimaryOutput().prettyPrint())); + throw new AssertionError("Neither branch cancelled the other one."); } else if (localResult != null) { return localResult; } else if (remoteResult != null) { @@ -507,6 +504,23 @@ ImmutableList callImpl(ActionExecutionContext context) DynamicSpawnStrategy.this.options, actionExecutionContext, spawn)); + } catch (DynamicInterruptedException e) { + // This exception can be thrown due to races in stopBranch(), in which case + // the branch that lost the race may not have been cancelled yet. Cancel it here + // to prevent the listener from cross-cancelling. + localBranch.cancel(true); + throw e; + } catch ( + @SuppressWarnings("InterruptedExceptionSwallowed") + Throwable e) { + if (options.debugSpawnScheduler) { + logger.atInfo().log( + "Local branch of %s failed with %s: '%s'", + spawn.getResourceOwner(), + e.getClass().getSimpleName(), + e.getMessage()); + } + throw e; } finally { localDone.release(); } @@ -554,6 +568,23 @@ public ImmutableList callImpl(ActionExecutionContext context) spawn)); delayLocalExecution.set(true); return spawnResults; + } catch (DynamicInterruptedException e) { + // This exception can be thrown due to races in stopBranch(), in which case + // the branch that lost the race may not have been cancelled yet. Cancel it here + // to prevent the listener from cross-cancelling. + remoteBranch.cancel(true); + throw e; + } catch ( + @SuppressWarnings("InterruptedExceptionSwallowed") + Throwable e) { + if (options.debugSpawnScheduler) { + logger.atInfo().log( + "Remote branch of %s failed with %s: '%s'", + spawn.getResourceOwner(), + e.getClass().getSimpleName(), + e.getMessage()); + } + throw e; } finally { remoteDone.release(); } diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/BUILD b/src/test/java/com/google/devtools/build/lib/dynamic/BUILD index 32011dd80df5a3..f142c1b00c0dae 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/BUILD +++ b/src/test/java/com/google/devtools/build/lib/dynamic/BUILD @@ -18,8 +18,11 @@ java_test( srcs = ["DynamicSpawnStrategyUnitTest.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/dynamic", "//src/main/java/com/google/devtools/build/lib/exec:execution_policy", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/protobuf:failure_details_java_proto", "//src/test/java/com/google/devtools/build/lib/exec/util", "//src/test/java/com/google/devtools/build/lib/testutil", diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java index 9e7412c8be6af7..8d63f04f66a4a2 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java @@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import static junit.framework.TestCase.fail; import static org.junit.Assert.assertThrows; @@ -571,6 +572,7 @@ public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalFailsLater() throw @Test public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() throws Exception { + assume().that(legacyBehavior).isFalse(); MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy"); MockSpawnStrategy remoteStrategy = @@ -592,6 +594,7 @@ public void actionSucceedsIfLocalExecutionSucceedsEvenIfRemoteRunsNothing() thro @Test public void actionSucceedsIfRemoteExecutionSucceedsEvenIfLocalRunsNothing() throws Exception { + assume().that(legacyBehavior).isFalse(); MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); @@ -723,6 +726,7 @@ public void actionFailsIfLocalAndRemoteFail() throws Exception { @Test public void actionFailsIfLocalAndRemoteRunNothing() throws Exception { + assume().that(legacyBehavior).isFalse(); MockSpawnStrategy localStrategy = new MockSpawnStrategy("MockLocalSpawnStrategy", DoExec.NOTHING, DoExec.NOTHING, false); diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java index 773f17a4a24bf0..fccdf442395f46 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyUnitTest.java @@ -28,8 +28,10 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; +import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.DynamicStrategyRegistry; import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy; import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy.StopConcurrentSpawns; @@ -41,8 +43,12 @@ import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.server.FailureDetails.Execution; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.testutil.TestFileOutErr; import com.google.devtools.build.lib.testutil.TestUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -85,8 +91,15 @@ private static void verifyNoInteractions(Object... objs) { @Mock private Function> mockGetPostProcessingSpawn; + private Scratch scratch; + private Path execDir; + private ArtifactRoot rootDir; + @Before - public void initMocks() { + public void initMocks() throws IOException { + scratch = new Scratch(); + execDir = scratch.dir("/base/exec"); + rootDir = ArtifactRoot.asDerivedRoot(execDir, "root"); MockitoAnnotations.initMocks(this); // Mockito can't see that we want the function to return Optional.empty() instead // 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 assertThat(results).containsExactly(SUCCESSFUL_SPAWN_RESULT, SUCCESSFUL_SPAWN_RESULT); } - @Test - public void waitBranches_givesDebugOutputOnWeirdCases() throws Exception { - Spawn spawn = - new SpawnBuilder() - .withOwnerPrimaryOutput(new SourceArtifact(rootDir, PathFragment.create("/foo"), null)) - .build(); - AssertionError error = - assertThrows( - AssertionError.class, - () -> - DynamicSpawnStrategy.waitBranches( - Futures.immediateFuture(null), Futures.immediateFuture(null), spawn)); - assertThat(error).hasMessageThat().contains("Neither branch of /foo completed."); - } - @Test public void exec_runAnywhereSpawn_localCantExec_runsRemote() throws Exception { Spawn spawn = new SpawnBuilder().build();