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, 33 insertions, 1 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
index a7ebc8f7a1..7647d17dd3 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
@@ -135,6 +135,7 @@ public class JavaSubprocessFactory implements SubprocessFactory {
 
     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.
diff --git a/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java b/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java
index 46d273fca2..d3ce1ee21b 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList;
 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 class SubprocessBuilder {
   private File stderrFile;
   private File workingDirectory;
   private long timeoutMillis;
+  private boolean redirectErrorStream;
 
   static SubprocessFactory factory = JavaSubprocessFactory.INSTANCE;
 
@@ -93,6 +95,11 @@ public class SubprocessBuilder {
     return this;
   }
 
+  public SubprocessBuilder setArgv(String... argv) {
+    this.setArgv(Arrays.asList(argv));
+    return this;
+  }
+
   public ImmutableMap<String, String> getEnv() {
     return env;
   }
@@ -171,7 +178,8 @@ public class SubprocessBuilder {
 
   /**
    * 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;
@@ -179,6 +187,29 @@ public class SubprocessBuilder {
     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;
   }