Check the result of Future.cancel() when cancelling the other branch of dynamic execution.

larsrc-google · copybara-github · commit 5b95d9162b56 · 2021-05-12T05:16:02.000-07:00
Under --experimental_local_lockfree_output, the local branch may succeed and set the future before cancelling the remote branch. If the remote branch succeeded after that but before the local listener got going, it would falsely think the local was cancelled, which would occasionally lead to strange errors.

RELNOTES: None.
PiperOrigin-RevId: 373347494
diff --git a/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java b/src/main/java/com/google/devtools/build/lib/actions/DynamicStrategyRegistry.java
@@ -34,6 +34,10 @@ enum DynamicMode {
     public String toString() {
       return name;
     }
+
+    public DynamicMode other() {
+      return this == REMOTE ? LOCAL : REMOTE;
+    }
   }
 
   /**
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
@@ -173,7 +173,17 @@ private static void stopBranch(
                           spawn.getMnemonic(), strategyThatCancelled.get())));
         }
 
-        branchToCancel.cancel(true);
+        if (!branchToCancel.cancel(true)) {
+          // This can happen if the other branch is local under local_lockfree and has returned
+          // its result but not yet cancelled this branch, or if the other branch was already
+          // cancelled for other reasons.
+          if (!branchToCancel.isCancelled()) {
+            throw new DynamicInterruptedException(
+                String.format(
+                    "Execution of %s strategy stopped because %s strategy could not be cancelled",
+                    cancellingStrategy, cancellingStrategy.other()));
+          }
+        }
         branchDone.acquire();
       } else {
         throw new DynamicInterruptedException(

Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,10 @@ enum DynamicMode {`
`34`	`34`	`public String toString() {`
`35`	`35`	`return name;`
`36`	`36`	`}`
	`37`	`+`
	`38`	`+ public DynamicMode other() {`
	`39`	`+ return this == REMOTE ? LOCAL : REMOTE;`
	`40`	`+ }`
`37`	`41`	`}`
`38`	`42`
`39`	`43`	`/**`