Fix bug in WorkRequestHandler's handling of singleplex requests that would cause occasional hangs.

larsrc-google · copybara-github · commit 5e352afe2b35 · 2021-07-22T02:26:38.000-07:00
RELNOTES: None.
PiperOrigin-RevId: 386194200
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkRequestHandler.java b/src/main/java/com/google/devtools/build/lib/worker/WorkRequestHandler.java
@@ -305,26 +305,40 @@ public WorkRequestHandler build() {
    * returns. If {@code in} reaches EOF, it also returns.
    */
   public void processRequests() throws IOException {
-    while (true) {
-      WorkRequest request = messageProcessor.readWorkRequest();
-      if (request == null) {
-        break;
-      }
-      if (request.getCancel()) {
-        respondToCancelRequest(request);
-      } else {
-        startResponseThread(request);
+    try {
+      while (true) {
+        WorkRequest request = messageProcessor.readWorkRequest();
+        if (request == null) {
+          break;
+        }
+        if (request.getCancel()) {
+          respondToCancelRequest(request);
+        } else {
+          startResponseThread(request);
+        }
       }
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      stderr.println("InterruptedException processing requests.");
     }
   }
 
   /** Starts a thread for the given request. */
-  void startResponseThread(WorkRequest request) {
+  void startResponseThread(WorkRequest request) throws InterruptedException {
     Thread currentThread = Thread.currentThread();
     String threadName =
         request.getRequestId() > 0
             ? "multiplex-request-" + request.getRequestId()
             : "singleplex-request";
+    // TODO(larsrc): See if this can be handled with a queue instead, without introducing more
+    // race conditions.
+    if (request.getRequestId() == 0) {
+      while (activeRequests.containsKey(request.getRequestId())) {
+        // b/194051480: Previous singleplex requests can still be in activeRequests for a bit after
+        // the response has been sent. We need to wait for them to vanish.
+        Thread.sleep(1);
+      }
+    }
     Thread t =
         new Thread(
             () -> {
diff --git a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java
@@ -98,7 +98,14 @@ public void processRequests() throws IOException {
         if (request.getCancel()) {
           respondToCancelRequest(request);
         } else {
-          startResponseThread(request);
+          try {
+            startResponseThread(request);
+          } catch (InterruptedException e) {
+            // We don't expect interrupts at this level, only inside the individual request
+            // handling threads, so here we just abort on interrupt.
+            e.printStackTrace();
+            return;
+          }
         }
         if (workerOptions.exitAfter > 0 && workUnitCounter > workerOptions.exitAfter) {
           System.exit(0);
