workers: Make sure to wait for worker processes to exit so that they don't become zombies.

--
MOS_MIGRATED_REVID=103541217

5 files changed, 45 insertions, 19 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/ThreadSafety.java b/src/main/java/com/google/devtools/build/lib/concurrent/ThreadSafety.java
index 0c67fd9666..8d8be179c3 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/ThreadSafety.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/ThreadSafety.java
@@ -24,7 +24,7 @@ import java.lang.annotation.Target;
  *<p>
  * The names used here are adapted from those used in Joshua Bloch's book
  * "Effective Java", which are also described at
- * <http://www-128.ibm.com/developerworks/java/library/j-jtp09263.html>.
+ * <http://www.ibm.com/developerworks/java/library/j-jtp09263/>.
  *<p>
  * These attributes are just documentation.  They don't have any run-time
  * effect.  The main aim is mainly just to standardize the terminology.
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Command.java b/src/main/java/com/google/devtools/build/lib/shell/Command.java
index 364373dcb5..b0ed4a5ded 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/Command.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/Command.java
@@ -846,8 +846,7 @@ public final class Command {
         }
       }
     } finally {
-      // Read this for detailed explanation:
-      // http://www-128.ibm.com/developerworks/java/library/j-jtp05236.html
+      // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/
       if (wasInterrupted) {
         Thread.currentThread().interrupt(); // preserve interrupted status
       }
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Consumers.java b/src/main/java/com/google/devtools/build/lib/shell/Consumers.java
index 2e548f6944..d6fe4d9f1e 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/Consumers.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/Consumers.java
@@ -260,7 +260,7 @@ class Consumers {
         }
       } finally {
         // Read this for detailed explanation:
-        // http://www-128.ibm.com/developerworks/java/library/j-jtp05236.html
+        // http://www.ibm.com/developerworks/java/library/j-jtp05236/
         if (wasInterrupted) {
           Thread.currentThread().interrupt(); // preserve interrupted status
         }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
index d76b9fe8f1..307761a226 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java
@@ -63,12 +63,13 @@ final class Worker {
 
     final Process process = processBuilder.start();
 
-    Thread shutdownHook = new Thread() {
-      @Override
-      public void run() {
-        process.destroy();
-      }
-    };
+    Thread shutdownHook =
+        new Thread() {
+          @Override
+          public void run() {
+            destroyProcess(process);
+          }
+        };
     Runtime.getRuntime().addShutdownHook(shutdownHook);
 
     if (verbose) {
@@ -87,7 +88,33 @@ final class Worker {
 
   void destroy() {
     Runtime.getRuntime().removeShutdownHook(shutdownHook);
-    process.destroy();
+    destroyProcess(process);
+  }
+
+  /**
+   * Destroys a process and waits for it to exit. This is necessary for the child to not become a
+   * zombie.
+   *
+   * @param process the process to destroy.
+   */
+  private static void destroyProcess(Process process) {
+    boolean wasInterrupted = false;
+    try {
+      process.destroy();
+      while (true) {
+        try {
+          process.waitFor();
+          return;
+        } catch (InterruptedException ie) {
+          wasInterrupted = true;
+        }
+      }
+    } finally {
+      // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/
+      if (wasInterrupted) {
+        Thread.currentThread().interrupt(); // preserve interrupted status
+      }
+    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
index 43553386d1..dd0508f512 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
@@ -148,20 +148,20 @@ final class WorkerSpawnStrategy implements SpawnActionContext {
             "Worker process did not return a correct WorkResponse. This is probably caused by a "
                 + "bug in the worker, writing unexpected other data to stdout.");
       }
-    } catch (InterruptedException e) {
-      // The user pressed Ctrl-C. Get out here quick.
-      if (worker != null) {
-        workers.invalidateObject(key, worker);
-        worker = null;
-      }
-      throw e;
     } catch (Exception e) {
-      // "Something" failed - let's retry with a fresh worker.
+      if (e instanceof InterruptedException) {
+        // The user pressed Ctrl-C. Get out here quick.
+        retriesLeft = 0;
+      }
+
       if (worker != null) {
         workers.invalidateObject(key, worker);
         worker = null;
       }
+
       if (retriesLeft > 0) {
+        // The worker process failed, but we still have some retries left. Let's retry with a fresh
+        // worker.
         eventHandler.handle(
             Event.warn(
                 key.getMnemonic()