Remote: Postpone the block waiting in afterCommand to BlockWaitingModule (bazelbuild#14833)

brentleyjones · coeuvre · web-flow · commit 0c7474174230 · 2022-02-17T03:28:45.000+11:00
When implementing async upload, we introduced a block waiting behaviour in `RemoteModule#afterCommand` so that uploads happened in the background can be waited before the whole build complete. However, there are other block waiting code in other module's `afterCommand` method (e.g. BES module). Block waiting in remote module will prevent other modules' `afterCommand` from executing until it's completed. This causes issues like bazelbuild#14576. This PR adds a new module `BlockWaitingModule` whose sole purpose is to accept tasks submitted by other modules in `afterCommand` and block waiting all the tasks in its own `afterCommand` method. So those tasks can be executed in parallel. This PR only updates RemoteModule's `afterCommand` method to submit block waiting task. Other modules should be updated to use `BlockWaitingModule` as well but that's beyond the scope this this PR. This PR along with 73a76a8 fix bazelbuild#14576. Closes bazelbuild#14618. PiperOrigin-RevId: 424295121 (cherry picked from commit 621649d) Co-authored-by: Chi Wang <chiwang@google.com>
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java b/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java
@@ -76,7 +76,10 @@ public final class Bazel {
           com.google.devtools.build.lib.metrics.PostGCMemoryUseRecorder.GcAfterBuildModule.class,
           com.google.devtools.build.lib.packages.metrics.PackageMetricsModule.class,
           com.google.devtools.build.lib.metrics.MetricsModule.class,
-          BazelBuiltinCommandModule.class);
+          BazelBuiltinCommandModule.class,
+          // This module needs to be registered after any module submitting tasks with its {@code
+          // submit} method.
+          com.google.devtools.build.lib.runtime.BlockWaitingModule.class);
 
   public static void main(String[] args) {
     BlazeVersionInfo.setBuildInfo(tryGetBuildInfo());
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingRepositoryLayoutResolver;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.runtime.BlockWaitingModule;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.vfs.Path;
 import java.util.concurrent.Executor;
@@ -211,7 +212,9 @@ void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
 
   public void afterCommand() {
     if (remoteExecutionService != null) {
-      remoteExecutionService.shutdown();
+      BlockWaitingModule blockWaitingModule =
+          checkNotNull(env.getRuntime().getBlazeModule(BlockWaitingModule.class));
+      blockWaitingModule.submit(() -> remoteExecutionService.shutdown());
     } else {
       if (remoteCache != null) {
         remoteCache.release();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlockWaitingModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlockWaitingModule.java
@@ -0,0 +1,59 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.runtime;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.devtools.build.lib.util.AbruptExitException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import javax.annotation.Nullable;
+
+/** A {@link BlazeModule} that waits for submitted tasks to terminate after every command. */
+public class BlockWaitingModule extends BlazeModule {
+  @Nullable private ExecutorService executorService;
+
+  @Override
+  public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
+    checkState(executorService == null, "executorService must be null");
+
+    executorService =
+        Executors.newCachedThreadPool(
+            new ThreadFactoryBuilder().setNameFormat("block-waiting-%d").build());
+  }
+
+  @SuppressWarnings("FutureReturnValueIgnored")
+  public void submit(Runnable task) {
+    checkNotNull(executorService, "executorService must not be null");
+
+    executorService.submit(task);
+  }
+
+  @Override
+  public void afterCommand() throws AbruptExitException {
+    checkNotNull(executorService, "executorService must not be null");
+
+    executorService.shutdown();
+    try {
+      executorService.awaitTermination(Long.MAX_VALUE, SECONDS);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+    }
+
+    executorService = null;
+  }
+}
