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 #2675

Change-Id: I1c9b1ac3c9383ed2fd28ea92f528f19649693275
PiperOrigin-RevId: 173244832

src/main/cpp/blaze_util_windows.cc

@@ -595,21 +595,10 @@ void ExecuteDaemon(const string& exe, const std::vector<string>& args_vector,
   CloseHandle(processInfo.hThread);
 }
 
-// Returns whether assigning the given process to a job failed because nested
-// jobs are not available on the current system.
-static bool IsFailureDueToNestedJobsNotSupported(HANDLE process) {
-  BOOL is_in_job;
-  if (!IsProcessInJob(process, NULL, &is_in_job)) {
-    pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "IsFailureDueToNestedJobsNotSupported: IsProcessInJob");
-    return false;
-  }
-
-  if (!is_in_job) {
-    // Not in a job.
-    return false;
-  }
-  return !IsWindows8OrGreater();
+// Returns whether nested jobs are not available on the current system.
+static bool NestedJobsSupported() {
+  // Nested jobs are supported from Windows 8
+  return IsWindows8OrGreater();
 }
 
 // Run the given program in the current working directory, using the given
@@ -624,20 +613,23 @@ void ExecuteProgram(const string& exe, const std::vector<string>& args_vector) {
 
   PROCESS_INFORMATION processInfo = {0};
 
-  HANDLE job = CreateJobObject(NULL, NULL);
-  if (job == NULL) {
-    pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "ExecuteProgram(%s): CreateJobObject", exe.c_str());
-  }
+  HANDLE job = INVALID_HANDLE_VALUE;
+  if (NestedJobsSupported()) {
+    job = CreateJobObject(NULL, NULL);
+    if (job == NULL) {
+      pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
+           "ExecuteProgram(%s): CreateJobObject", exe.c_str());
+    }
 
-  JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = {0};
-  job_info.BasicLimitInformation.LimitFlags =
-      JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+    JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = {0};
+    job_info.BasicLimitInformation.LimitFlags =
+        JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
 
-  if (!SetInformationJobObject(job, JobObjectExtendedLimitInformation,
-                               &job_info, sizeof(job_info))) {
-    pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "ExecuteProgram(%s): SetInformationJobObject", exe.c_str());
+    if (!SetInformationJobObject(job, JobObjectExtendedLimitInformation,
+                                 &job_info, sizeof(job_info))) {
+      pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
+           "ExecuteProgram(%s): SetInformationJobObject", exe.c_str());
+    }
   }
 
   BOOL success = CreateProcessA(
@@ -657,18 +649,23 @@ void ExecuteProgram(const string& exe, const std::vector<string>& args_vector) {
          "ExecuteProgram(%s): CreateProcess(%s)", exe.c_str(), cmdline.cmdline);
   }
 
-  // We will try to put the launched process into a Job object. This will make
-  // Windows reliably kill all child processes that the process itself may
-  // launch once the process exits. On Windows systems that don't support nested
-  // jobs, this may fail if we are already running inside a job ourselves. In
-  // this case, we'll continue anyway, because we assume that our parent is
-  // handling process management for us.
-  if (!AssignProcessToJobObject(job, processInfo.hProcess) &&
-      !IsFailureDueToNestedJobsNotSupported(processInfo.hProcess)) {
-    pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "ExecuteProgram(%s): AssignProcessToJobObject", exe.c_str());
+  // On Windows versions that support nested jobs (Windows 8 and above), we
+  // assign the Bazel server to a job object. Every process that Bazel creates,
+  // as well as all their child processes, will be assigned to this job object.
+  // When the Bazel server terminates the OS can reliably kill the entire
+  // process tree under it. On Windows versions that don't support nested jobs
+  // (Windows 7), we don't assign the Bazel server to a big job object. Instead,
+  // when Bazel creates new processes, it does so using the JNI library. The
+  // library assigns individual job objects to each subprocess. This way when
+  // these processes terminate, the OS can kill all their subprocesses. Bazel's
+  // own subprocesses are not in a job object though, so we only create
+  // subprocesses via the JNI library.
+  if (job != INVALID_HANDLE_VALUE) {
+    if (!AssignProcessToJobObject(job, processInfo.hProcess)) {
+      pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
+           "ExecuteProgram(%s): AssignProcessToJobObject", exe.c_str());
+    }
   }
-
   // Now that we potentially put the process into a new job object, we can start
   // running it.
   if (ResumeThread(processInfo.hThread) == -1) {

src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java

@@ -135,6 +135,7 @@ public Subprocess create(SubprocessBuilder params) throws IOException {
 
     builder.redirectOutput(getRedirect(params.getStdout(), params.getStdoutFile()));
     builder.redirectError(getRedirect(params.getStderr(), params.getStderrFile()));
+    builder.redirectErrorStream(params.redirectErrorStream());
     builder.directory(params.getWorkingDirectory());
 
     // Deadline is now + given timeout.

src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableMap;
 import java.io.File;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -48,6 +49,7 @@ public enum StreamAction {
   private File stderrFile;
   private File workingDirectory;
   private long timeoutMillis;
+  private boolean redirectErrorStream;
 
   static SubprocessFactory factory = JavaSubprocessFactory.INSTANCE;
 
@@ -93,6 +95,11 @@ public SubprocessBuilder setArgv(Iterable<String> argv) {
     return this;
   }
 
+  public SubprocessBuilder setArgv(String... argv) {
+    this.setArgv(Arrays.asList(argv));
+    return this;
+  }
+
   public ImmutableMap<String, String> getEnv() {
     return env;
   }
@@ -171,14 +178,38 @@ public SubprocessBuilder setStderr(StreamAction action) {
 
   /**
    * Sets the file stderr is appended to. If null, the stderr will be available as an input stream
-   * on the resulting object representing the process.
+   * on the resulting object representing the process. When {@code redirectErrorStream} is set to
+   * True, this method has no effect.
    */
   public SubprocessBuilder setStderr(File file) {
     this.stderrAction = StreamAction.REDIRECT;
     this.stderrFile = file;
     return this;
   }
 
+  /**
+   * Tells whether this process builder merges standard error and standard output.
+   *
+   * @return this process builder's {@code redirectErrorStream} property
+   */
+  public boolean redirectErrorStream() {
+    return redirectErrorStream;
+  }
+
+  /**
+   * Sets whether this process builder merges standard error and standard output.
+   *
+   * <p>If this property is {@code true}, then any error output generated by subprocesses
+   * subsequently started by this object's {@link #start()} method will be merged with the standard
+   * output, so that both can be read using the {@link Subprocess#getInputStream()} method. This
+   * makes it easier to correlate error messages with the corresponding output. The initial value is
+   * {@code false}.
+   */
+  public SubprocessBuilder redirectErrorStream(boolean redirectErrorStream) {
+    this.redirectErrorStream = redirectErrorStream;
+    return this;
+  }
+
   public File getWorkingDirectory() {
     return workingDirectory;
   }

src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java

@@ -52,7 +52,13 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
 
     long nativeProcess =
         WindowsProcesses.createProcess(
-            argv0, argvRest, env, builder.getWorkingDirectory().getPath(), stdoutPath, stderrPath);
+            argv0,
+            argvRest,
+            env,
+            builder.getWorkingDirectory().getPath(),
+            stdoutPath,
+            stderrPath,
+            builder.redirectErrorStream());
     String error = WindowsProcesses.processGetLastError(nativeProcess);
     if (!error.isEmpty()) {
       WindowsProcesses.deleteProcess(nativeProcess);

src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java

@@ -33,24 +33,43 @@ private WindowsProcesses() {
    *     Windows path with a drive letter (e.g. "c:\foo\bar app.exe") or a single file name (e.g.
    *     "foo app.exe")
    * @param argvRest the rest of the command line, i.e. argv[1:] (needs to be quoted Windows style)
-   * @param commandLine the command line (needs to be quoted Windows style)
    * @param env the environment of the new process. null means inherit that of the Bazel server
    * @param cwd the working directory of the new process. if null, the same as that of the current
    *     process
    * @param stdoutFile the file the stdout should be redirected to. if null, nativeReadStdout will
    *     work.
    * @param stderrFile the file the stdout should be redirected to. if null, nativeReadStderr will
    *     work.
+   * @param redirectErrorStream whether we merge the process's standard error and standard output.
    * @return the opaque identifier of the created process
    */
+  public static long createProcess(
+      String argv0,
+      String argvRest,
+      byte[] env,
+      String cwd,
+      String stdoutFile,
+      String stderrFile,
+      boolean redirectErrorStream) {
+    WindowsJniLoader.loadJni();
+    return nativeCreateProcess(
+        argv0, argvRest, env, cwd, stdoutFile, stderrFile, redirectErrorStream);
+  }
+
   public static long createProcess(
       String argv0, String argvRest, byte[] env, String cwd, String stdoutFile, String stderrFile) {
     WindowsJniLoader.loadJni();
-    return nativeCreateProcess(argv0, argvRest, env, cwd, stdoutFile, stderrFile);
+    return nativeCreateProcess(argv0, argvRest, env, cwd, stdoutFile, stderrFile, false);
   }
 
   private static native long nativeCreateProcess(
-      String argv0, String argvRest, byte[] env, String cwd, String stdoutFile, String stderrFile);
+      String argv0,
+      String argvRest,
+      byte[] env,
+      String cwd,
+      String stdoutFile,
+      String stderrFile,
+      boolean redirectErrorStream);
 
   /**
    * Writes data from the given array to the stdin of the specified process.

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",

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 @@ public void run() {
   }
 
   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 @@ void destroy() throws IOException {
    *
    * @param process the process to destroy.
    */
-  private static void destroyProcess(Process process) {
+  private static void destroyProcess(Subprocess process) {
     boolean wasInterrupted = false;
     try {
       process.destroy();