From 7d9d23c1ac1b7fcaa461f902e286f50fbb7cb116 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 23 Feb 2023 09:40:26 -0800 Subject: [PATCH] Correctly set default subprocess factory when loading class `SubprocessBuilder`. 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 --- .../credentialhelper/CredentialHelper.java | 5 ++++- .../build/lib/runtime/BlazeRuntime.java | 15 --------------- .../com/google/devtools/build/lib/shell/BUILD | 10 ++++++++++ .../build/lib/shell/SubprocessBuilder.java | 17 +++++++++++++++-- .../{windows => shell}/WindowsSubprocess.java | 4 ++-- .../WindowsSubprocessFactory.java | 13 +++++++------ .../com/google/devtools/build/lib/windows/BUILD | 5 ----- .../lib/windows/WindowsSubprocessTest.java | 2 ++ 8 files changed, 40 insertions(+), 31 deletions(-) rename src/main/java/com/google/devtools/build/lib/{windows => shell}/WindowsSubprocess.java (98%) rename src/main/java/com/google/devtools/build/lib/{windows => shell}/WindowsSubprocessFactory.java (94%) diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java index c1f0a09f12bb7f..f5e27bd4c4aca5 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java +++ b/src/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.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 index 4b6d64ec871d76..d79a509bf990b7 100644 --- 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 index fedc350a50e5b2..6ea795813746e9 100644 --- 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 index 266c52e7864661..09e168381650a0 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 @@ -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/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java b/src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocess.java similarity index 98% rename from src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java rename to src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocess.java index 2b7bf474f051d7..dbe3a23c414553 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java +++ b/src/main/java/com/google/devtools/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/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocessFactory.java similarity index 94% rename from src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java rename to src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocessFactory.java index 6c56710224f94f..55d7d288e958d5 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java +++ b/src/main/java/com/google/devtools/build/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 index ff57f9a34f4218..54805ee8011f9b 100644 --- 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 index 1ac68f6c341e15..1248fba3a1fd1d 100644 --- 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;