Correctly set default subprocess factory when loading class `Subproce…

…ssBuilder`. Currently, there are two subprocess factories: `JavaSubprocessFactory` and `WindowsSubprocessFactory`. The default factory is set to `JavaSubprocessFactory` when loading class `SubprocessBuidler`. We call `setDefaultSubprocessFactory` to set the default factory, when creating BlazeRuntime, by checking the OS, which set the default factory to WindowsSubprocessFactory on Windows. However, for integration tests written in Java, `setDefaultSubprocessFactory` is not called. So we use `JavaSubprocessFactory` even on Windows for these tests which doesn't correctly terminate process tree. This CL fixes that by calling `setDefaultSubprocessFactory` when loading class `SubprocessBuilder`. PiperOrigin-RevId: 511810605 Change-Id: I55ca32caa0f81160e027780d671bf46a6bd6c266
fweikert · Feb 23, 2023 · 7d9d23c · 7d9d23c
1 parent 22ae3a6
commit 7d9d23c
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 31 deletions.
diff --git a/...main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/...main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java
@@ -23,6 +23,7 @@
 import com.google.common.io.CharStreams;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
+import com.google.devtools.build.lib.shell.JavaSubprocessFactory;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.vfs.Path;
@@ -145,7 +146,9 @@ private Subprocess spawnSubprocess(CredentialHelperEnvironment environment, Stri
     Preconditions.checkNotNull(environment);
     Preconditions.checkNotNull(args);
 
-    return new SubprocessBuilder()
+    // Force using JavaSubprocessFactory on Windows, because for some reasons,
+    // WindowsSubprocessFactory cannot redirect stdin to subprocess.
+    return new SubprocessBuilder(JavaSubprocessFactory.INSTANCE)
         .setArgv(ImmutableList.<String>builder().add(path.getPathString()).add(args).build())
         .setWorkingDirectory(environment.getWorkspacePath().getPathFile())
         .setEnv(environment.getClientEnvironment())

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -84,9 +84,6 @@
 import com.google.devtools.build.lib.server.RPCServer;
 import com.google.devtools.build.lib.server.ShutdownHooks;
 import com.google.devtools.build.lib.server.signal.InterruptSignalHandler;
-import com.google.devtools.build.lib.shell.JavaSubprocessFactory;
-import com.google.devtools.build.lib.shell.SubprocessBuilder;
-import com.google.devtools.build.lib.shell.SubprocessFactory;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.CustomExitCodePublisher;
 import com.google.devtools.build.lib.util.CustomFailureDetailPublisher;
@@ -95,7 +92,6 @@
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.InterruptedFailureDetails;
 import com.google.devtools.build.lib.util.LoggingUtil;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.ProcessUtils;
 import com.google.devtools.build.lib.util.TestType;
@@ -105,7 +101,6 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.windows.WindowsSubprocessFactory;
 import com.google.devtools.build.lib.worker.WorkerMetricsCollector;
 import com.google.devtools.common.options.CommandNameCache;
 import com.google.devtools.common.options.InvocationPolicyParser;
@@ -1090,14 +1085,6 @@ public void run() {
     }
   }
 
-  private static SubprocessFactory subprocessFactoryImplementation() {
-    if (JniLoader.isJniAvailable() && OS.getCurrent() == OS.WINDOWS) {
-      return WindowsSubprocessFactory.INSTANCE;
-    } else {
-      return JavaSubprocessFactory.INSTANCE;
-    }
-  }
-
   /**
    * Parses the command line arguments into a {@link OptionsParser} object.
    *
@@ -1257,8 +1244,6 @@ private static BlazeRuntime newRuntime(
               return null;
             });
 
-    SubprocessBuilder.setDefaultSubprocessFactory(subprocessFactoryImplementation());
-
     Path outputUserRootPath = fs.getPath(outputUserRoot);
     Path installBasePath = fs.getPath(installBase);
     Path outputBasePath = fs.getPath(outputBase);

diff --git a/src/main/java/com/google/devtools/build/lib/shell/BUILD b/src/main/java/com/google/devtools/build/lib/shell/BUILD
@@ -17,8 +17,12 @@ java_library(
     name = "shell",
     srcs = glob(["*.java"]),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/jni",
         "//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/lib/windows:processes",
         "//src/main/protobuf:execution_statistics_java_proto",
         "//third_party:auto_value",
         "//third_party:flogger",
@@ -36,7 +40,13 @@ bootstrap_java_library(
         exclude = ["ExecutionStatistics.java"],
     ),
     jars = [
+        "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
+        "//src/main/java/com/google/devtools/build/lib/jni",
         "//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
+        "//src/main/java/com/google/devtools/build/lib/util:filetype",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/lib/windows:processes",
         "//third_party:auto_value-jars",
         "//third_party:flogger-jars",
         "//third_party:bootstrap_guava_and_error_prone-jars",

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
@@ -14,9 +14,12 @@
 
 package com.google.devtools.build.lib.shell;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.jni.JniLoader;
+import com.google.devtools.build.lib.util.OS;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.io.File;
 import java.io.IOException;
@@ -52,14 +55,24 @@ public enum StreamAction {
   private long timeoutMillis;
   private boolean redirectErrorStream;
 
-  static SubprocessFactory defaultFactory = JavaSubprocessFactory.INSTANCE;
+  static SubprocessFactory defaultFactory = subprocessFactoryImplementation();
+
+  private static SubprocessFactory subprocessFactoryImplementation() {
+    if (JniLoader.isJniAvailable() && OS.getCurrent() == OS.WINDOWS) {
+      return WindowsSubprocessFactory.INSTANCE;
+    } else {
+      return JavaSubprocessFactory.INSTANCE;
+    }
+  }
 
   /**
    * Sets the default factory class for creating subprocesses. Passing {@code null} resets it to the
    * initial state.
    */
+  @VisibleForTesting
   public static void setDefaultSubprocessFactory(SubprocessFactory factory) {
-    SubprocessBuilder.defaultFactory = factory != null ? factory : JavaSubprocessFactory.INSTANCE;
+    SubprocessBuilder.defaultFactory =
+        factory != null ? factory : subprocessFactoryImplementation();
   }
 
   public SubprocessBuilder() {

diff --git a/.../build/lib/windows/WindowsSubprocess.java → ...ls/build/lib/shell/WindowsSubprocess.java b/.../build/lib/windows/WindowsSubprocess.java → ...ls/build/lib/shell/WindowsSubprocess.java
@@ -12,10 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.devtools.build.lib.windows;
+package com.google.devtools.build.lib.shell;
 
 import com.google.common.base.Throwables;
-import com.google.devtools.build.lib.shell.Subprocess;
+import com.google.devtools.build.lib.windows.WindowsProcesses;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;

diff --git a/...lib/windows/WindowsSubprocessFactory.java → ...d/lib/shell/WindowsSubprocessFactory.java b/...lib/windows/WindowsSubprocessFactory.java → ...d/lib/shell/WindowsSubprocessFactory.java
@@ -12,14 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.devtools.build.lib.windows;
+package com.google.devtools.build.lib.shell;
 
-import com.google.devtools.build.lib.shell.ShellUtils;
-import com.google.devtools.build.lib.shell.Subprocess;
-import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
-import com.google.devtools.build.lib.shell.SubprocessFactory;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.windows.WindowsProcesses;
 import java.io.File;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
@@ -44,6 +41,10 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
             : "";
     byte[] env = convertEnvToNative(builder.getEnv());
 
+    String cwd = null;
+    if (builder.getWorkingDirectory() != null) {
+      cwd = builder.getWorkingDirectory().getPath();
+    }
     String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile());
     String stderrPath = getRedirectPath(builder.getStderr(), builder.getStderrFile());
 
@@ -52,7 +53,7 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
             argv0,
             argvRest,
             env,
-            builder.getWorkingDirectory().getPath(),
+            cwd,
             stdoutPath,
             stderrPath,
             builder.redirectErrorStream());

diff --git a/src/main/java/com/google/devtools/build/lib/windows/BUILD b/src/main/java/com/google/devtools/build/lib/windows/BUILD
@@ -30,18 +30,13 @@ java_library(
     name = "windows",
     srcs = [
         "WindowsFileSystem.java",
-        "WindowsSubprocess.java",
-        "WindowsSubprocessFactory.java",
     ],
     deps = [
         ":file",
-        ":processes",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/profiler",
-        "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",
-        "//third_party:jsr305",
     ],
 )
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java
@@ -23,6 +23,8 @@
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
+import com.google.devtools.build.lib.shell.WindowsSubprocess;
+import com.google.devtools.build.lib.shell.WindowsSubprocessFactory;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.runfiles.Runfiles;