Windows, jni: Don't close stdout/stderr in nativeWaitFor function

These two close operations were added to work around #1708, but caused #2675.

We found the root cause of the hanging problem in #1708 is a race
condition when creating Windows processes:

When Bazel trys to create two processes, one for a local command
execution, one for starting the worker process. The worker process
might accidentally inherits handles opened when creating the local
command process, and it holds those handles as long as it lives.
Therefore, ReadFile function hangs when handles for the write end of
stdout/stderr pipes are released by the worker.

The solution is to make Bazel native createProcess JNI function
explicitly inheirts handles as needed, and use this function to start
worker process.

Related: http://support.microsoft.com/kb/315939
Fixed https://github.com/bazelbuild/bazel/issues/2675

Change-Id: I1c9b1ac3c9383ed2fd28ea92f528f19649693275
PiperOrigin-RevId: 173244832

2 files changed, 16 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD
index bbe49c027a..ae68799d72 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD
@@ -24,6 +24,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/sandbox",
+        "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/common/options",
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 0268d1b2b1..55705f4485 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
@@ -14,13 +14,16 @@
 package com.google.devtools.build.lib.worker;
 
 import com.google.common.hash.HashCode;
+import com.google.devtools.build.lib.shell.Subprocess;
+import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.lang.ProcessBuilder.Redirect;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.SortedMap;
 
 /**
@@ -40,7 +43,7 @@ class Worker {
   private final Path workDir;
   private final Path logFile;
 
-  private Process process;
+  private Subprocess process;
   private Thread shutdownHook;
 
   Worker(WorkerKey workerKey, int workerId, final Path workDir, Path logFile) {
@@ -66,19 +69,17 @@ class Worker {
   }
 
   void createProcess() throws IOException {
-    String[] command = workerKey.getArgs().toArray(new String[0]);
-
-    // Follows the logic of {@link com.google.devtools.build.lib.shell.Command}.
-    File executable = new File(command[0]);
+    List<String> args = workerKey.getArgs();
+    File executable = new File(args.get(0));
     if (!executable.isAbsolute() && executable.getParent() != null) {
-      command[0] = new File(workDir.getPathFile(), command[0]).getAbsolutePath();
+      args = new ArrayList<>(args);
+      args.set(0, new File(workDir.getPathFile(), args.get(0)).getAbsolutePath());
     }
-    ProcessBuilder processBuilder =
-        new ProcessBuilder(command)
-            .directory(workDir.getPathFile())
-            .redirectError(Redirect.appendTo(logFile.getPathFile()));
-    processBuilder.environment().clear();
-    processBuilder.environment().putAll(workerKey.getEnv());
+    SubprocessBuilder processBuilder = new SubprocessBuilder();
+    processBuilder.setArgv(args);
+    processBuilder.setWorkingDirectory(workDir.getPathFile());
+    processBuilder.setStderr(logFile.getPathFile());
+    processBuilder.setEnv(workerKey.getEnv());
 
     this.process = processBuilder.start();
   }
@@ -98,7 +99,7 @@ class Worker {
    *
    * @param process the process to destroy.
    */
-  private static void destroyProcess(Process process) {
+  private static void destroyProcess(Subprocess process) {
     boolean wasInterrupted = false;
     try {
       process.destroy();