Extract common SpawnContinuation handling

Extract the SpawnContinuation that is used to start an action to the SpawnContinuation class. Progress on #6394. PiperOrigin-RevId: 240526086
bazelbuild · Mar 27, 2019 · 3bd9a10 · 3bd9a10
1 parent 2029979
commit 3bd9a10
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 79 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnContinuation.java
@@ -43,6 +43,45 @@ public static SpawnContinuation immediate(List<SpawnResult> spawnResults) {
     return new Finished(ImmutableList.copyOf(spawnResults));
   }
 
+  /**
+   * Returns a SpawnContinuation implementation that calls {@link SpawnActionContext#beginExecution}
+   * on a {@link SpawnActionContext} instance obtained from the {@link ActionExecutionContext}. This
+   * continuation does not have a future, and will actually throw {@link IllegalStateException} when
+   * {@link #getFuture} is called. The intended pattern of use is:
+   *
+   * <pre>
+   *   SpawnContinuation spawnContinuation =
+   *         SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext);
+   *   return new CppLinkActionContinuation(actionExecutionContext, spawnContinuation).execute();
+   * </pre>
+   *
+   * <p>This ensures that the action-specific exception handling for {@link #execute} is centralized
+   * in one location.
+   */
+  public static SpawnContinuation ofBeginExecution(
+      Spawn spawn, ActionExecutionContext actionExecutionContext) {
+    return new SpawnContinuation() {
+      @Override
+      public ListenableFuture<?> getFuture() {
+        // TODO(ulfjack): This is technically a violation of the SpawnContinuation contract, which
+        //  requires a non-null value when isDone() returns false. We use this to avoid having to
+        //  duplicate the exception handling wrapping the execute() call. Clients are supposed to
+        //  immediately call ActionContinuationOrResult.execute(), which immediately calls
+        //  SpawnContinuation.execute() without checking interface consistency. We should either
+        //  clarify in SpawnContinuation that this is a legal use, or refactor the code to avoid
+        //  this, e.g., by extracting the exception handling code in some other way.
+        throw new IllegalStateException();
+      }
+
+      @Override
+      public SpawnContinuation execute() throws ExecException, InterruptedException {
+        return actionExecutionContext
+            .getContext(SpawnActionContext.class)
+            .beginExecution(spawn, actionExecutionContext);
+      }
+    };
+  }
+
   /**
    * Runs the state machine represented by the given continuation to completion, blocking as
    * necessary until all asynchronous computations finish, and the final continuation is done. Then

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -306,19 +306,7 @@ public final ActionContinuationOrResult beginExecution(
     SpawnActionContinuation continuation =
         new SpawnActionContinuation(
             actionExecutionContext,
-            new SpawnContinuation() {
-              @Override
-              public ListenableFuture<?> getFuture() {
-                return null;
-              }
-
-              @Override
-              public SpawnContinuation execute() throws ExecException, InterruptedException {
-                SpawnActionContext context =
-                    actionExecutionContext.getContext(SpawnActionContext.class);
-                return context.beginExecution(spawn, actionExecutionContext);
-              }
-            });
+            SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext));
     return continuation.execute();
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -255,20 +255,8 @@ private TestAttemptContinuation beginTestAttempt(
             testOutErr,
             streamed,
             startTimeMillis,
-            new SpawnContinuation() {
-              @Override
-              public ListenableFuture<?> getFuture() {
-                return null;
-              }
-
-              @Override
-              public SpawnContinuation execute() throws ExecException, InterruptedException {
-                SpawnActionContext spawnActionContext =
-                    actionExecutionContext.getContext(SpawnActionContext.class);
-                return spawnActionContext.beginExecution(
-                    spawn, actionExecutionContext.withFileOutErr(testOutErr));
-              }
-            })
+            SpawnContinuation.ofBeginExecution(
+                spawn, actionExecutionContext.withFileOutErr(testOutErr)))
         .execute();
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -1177,20 +1177,7 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
             spawnContext,
             showIncludesFilterForStdout,
             showIncludesFilterForStderr,
-            new SpawnContinuation() {
-              @Override
-              public ListenableFuture<?> getFuture() {
-                // This shouldn't be called from CppCompileActionContinuation.
-                throw new IllegalStateException();
-              }
-
-              @Override
-              public SpawnContinuation execute() throws ExecException, InterruptedException {
-                SpawnActionContext context =
-                    actionExecutionContext.getContext(SpawnActionContext.class);
-                return context.beginExecution(spawn, spawnContext);
-              }
-            })
+            SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext))
         .execute();
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -42,7 +42,6 @@
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.SimpleSpawn;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.SpawnContinuation;
 import com.google.devtools.build.lib.actions.extra.CppLinkInfo;
 import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -292,30 +291,9 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
       return ActionContinuationOrResult.of(ActionResult.EMPTY);
     }
     Spawn spawn = createSpawn(actionExecutionContext);
-    return new CppLinkActionContinuation(
-            actionExecutionContext,
-            new SpawnContinuation() {
-              @Override
-              public ListenableFuture<?> getFuture() {
-                // TODO(ulfjack): This is technically a violation of the SpawnContinuation contract,
-                //  which requires a non-null value when isDone() returns false. We use this to
-                //  avoid having to duplicate the exception handling wrapping the execute() call.
-                //  We call CppLinkActionContinuation.execute() below, which immediately calls
-                //  SpawnContinuation.execute() below without checking interface consistency. We
-                //  should either clarify in SpawnContinuation that this is a legal use, or refactor
-                //  the code to avoid this, e.g., by extracting the exception handling code in some
-                //  other way.
-                throw new IllegalStateException();
-              }
-
-              @Override
-              public SpawnContinuation execute() throws ExecException, InterruptedException {
-                return actionExecutionContext
-                    .getContext(SpawnActionContext.class)
-                    .beginExecution(spawn, actionExecutionContext);
-              }
-            })
-        .execute();
+    SpawnContinuation spawnContinuation =
+        SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext);
+    return new CppLinkActionContinuation(actionExecutionContext, spawnContinuation).execute();
   }
 
   @Override

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
@@ -303,19 +303,7 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
         new JavaActionContinuation(
             actionExecutionContext,
             reducedClasspath,
-            new SpawnContinuation() {
-              @Override
-              public ListenableFuture<?> getFuture() {
-                return null;
-              }
-
-              @Override
-              public SpawnContinuation execute() throws ExecException, InterruptedException {
-                SpawnActionContext context =
-                    actionExecutionContext.getContext(SpawnActionContext.class);
-                return context.beginExecution(spawn, actionExecutionContext);
-              }
-            });
+            SpawnContinuation.ofBeginExecution(spawn, actionExecutionContext));
     return continuation.execute();
   }