Ensure NetworkTime never throw exceptions inside onClose

Failed doing so will cause gRPC hanging forever. This could be one of causes that leads to bazelbuild#11782. Closes bazelbuild#12422. PiperOrigin-RevId: 340995977
coeuvre · Jan 28, 2021 · 6cd8bc3 · 6cd8bc3
1 parent 4827fc6
commit 6cd8bc3
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 2 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java b/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java
@@ -86,7 +86,7 @@ private Builder() {
     public Builder maxNumUniqueValues(int maxNumUniqueValues) {
       Preconditions.checkState(
           maxNumUniqueValues > 0,
-          "maxNumUniqueValues must be positive (was %d)",
+          "maxNumUniqueValues must be positive (was %s)",
           maxNumUniqueValues);
       this.maxNumUniqueValues = maxNumUniqueValues;
       return this;

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/BUILD b/src/main/java/com/google/devtools/build/lib/remote/util/BUILD
@@ -21,6 +21,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/options",
         "//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",
+        "//third_party:flogger",
         "//third_party:guava",
         "//third_party:jsr305",
         "//third_party/grpc:grpc-jar",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/NetworkTime.java b/src/main/java/com/google/devtools/build/lib/remote/util/NetworkTime.java
@@ -15,6 +15,7 @@
 
 import build.bazel.remote.execution.v2.ExecutionGrpc;
 import com.google.common.base.Stopwatch;
+import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import io.grpc.CallOptions;
 import io.grpc.Channel;
@@ -31,6 +32,8 @@
 /** Reentrant wall clock stopwatch and grpc interceptor for network waits. */
 @ThreadSafety.ThreadSafe
 public class NetworkTime {
+  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+
   public static final Context.Key<NetworkTime> CONTEXT_KEY = Context.key("remote-network-time");
 
   private final Stopwatch wallTime = Stopwatch.createUnstarted();
@@ -87,9 +90,20 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
           new ForwardingClientCallListener.SimpleForwardingClientCallListener<RespT>(
               responseListener) {
 
+            /**
+             * This method must not throw any exceptions. Doing so will cause the wrapped call to
+             * silently hang indefinitely: https://github.com/grpc/grpc-java/pull/6107
+             */
             @Override
             public void onClose(Status status, Metadata trailers) {
-              networkTime.stop();
+              // There is a risk that networkTime.stop() would throw a IllegalStateException: if
+              // networkTime.outstanding is overflowed, wallTime.stop() will be called even it's
+              // already stopped.
+              try {
+                networkTime.stop();
+              } catch (RuntimeException e) {
+                logger.atWarning().withCause(e).log("Failed to stop networkTime");
+              }
               super.onClose(status, trailers);
             }
           },