Skip to content

Commit

Permalink
subprocesses: customizable SubprocessFactory
Browse files Browse the repository at this point in the history 
Individual SubprocessBuilder instances can now use
a SubprocessFactory object other than the static
SubprocessBuilder.factory.

Also, WindowsSubprocessFactory is no longer a
singleton because it now stores state: whether to
use windows-style argument escaping or to use the
(broken) Bash-style escaping (causing #7122).

These two changes allow:
- a safer way to mock out SubprocessFactory in
  tests, because it's no longer necessary to
  change global state (i.e. the static
  SubprocessBuilder.factory member)
- testing old and new argument escaping semantics
  in WindowsSubprocessTest
- adding a flag that switches between old and new
  semantics in the SubprocessFactory, and thus
  allows rolling out the bugfix with just a flag
  flip

See #7122

PiperOrigin-RevId: 234105692
  • Loading branch information
@laszlocsomor @copybara-github
laszlocsomor authored and copybara-github committed Feb 15, 2019
1 parent fb016b6 commit 1da4861
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 49 deletions.
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1048,7 +1048,7 @@ private static FileSystem defaultFileSystemImplementation()

private static SubprocessFactory subprocessFactoryImplementation() {
if (!"0".equals(System.getProperty("io.bazel.EnableJni")) && OS.getCurrent() == OS.WINDOWS) {
return WindowsSubprocessFactory.INSTANCE;
return new WindowsSubprocessFactory(false);
} else {
return JavaSubprocessFactory.INSTANCE;
}
Expand Down Expand Up @@ -1155,7 +1155,7 @@ private static BlazeRuntime newRuntime(Iterable<BlazeModule> blazeModules, List<
"No module set the default hash function.", ExitCode.BLAZE_INTERNAL_ERROR, e);
}
Path.setFileSystemForSerialization(fs);
SubprocessBuilder.setSubprocessFactory(subprocessFactoryImplementation());
SubprocessBuilder.setDefaultSubprocessFactory(subprocessFactoryImplementation());

Path outputUserRootPath = fs.getPath(outputUserRoot);
Path installBasePath = fs.getPath(installBase);
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public enum StreamAction {
STREAM
}

private final SubprocessFactory factory;
private ImmutableList<String> argv;
private ImmutableMap<String, String> env;
private StreamAction stdoutAction;
Expand All @@ -51,15 +52,20 @@ public enum StreamAction {
private long timeoutMillis;
private boolean redirectErrorStream;

static SubprocessFactory factory = JavaSubprocessFactory.INSTANCE;
static SubprocessFactory defaultFactory = JavaSubprocessFactory.INSTANCE;

public static void setSubprocessFactory(SubprocessFactory factory) {
SubprocessBuilder.factory = factory;
public static void setDefaultSubprocessFactory(SubprocessFactory factory) {
SubprocessBuilder.defaultFactory = factory;
}

public SubprocessBuilder() {
this(defaultFactory);
}

public SubprocessBuilder(SubprocessFactory factory) {
stdoutAction = StreamAction.STREAM;
stderrAction = StreamAction.STREAM;
this.factory = factory;
}

/**
Expand Down
40 changes: 28 additions & 12 deletions src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.windows;

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;
Expand All @@ -31,10 +32,10 @@
* A subprocess factory that uses the Win32 API.
*/
public class WindowsSubprocessFactory implements SubprocessFactory {
public static final WindowsSubprocessFactory INSTANCE = new WindowsSubprocessFactory();
private final boolean windowsStyleArgEscaping;

private WindowsSubprocessFactory() {
// Singleton
public WindowsSubprocessFactory(boolean windowsStyleArgEscaping) {
this.windowsStyleArgEscaping = windowsStyleArgEscaping;
}

@Override
Expand All @@ -43,8 +44,7 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {

// DO NOT quote argv0, createProcess will do it for us.
String argv0 = processArgv0(argv.get(0));
String argvRest =
argv.size() > 1 ? WindowsProcesses.quoteCommandLine(argv.subList(1, argv.size())) : "";
String argvRest = argv.size() > 1 ? escapeArgvRest(argv.subList(1, argv.size())) : "";
byte[] env = convertEnvToNative(builder.getEnv());

String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile());
Expand Down Expand Up @@ -73,7 +73,25 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
builder.getTimeoutMillis());
}

public String processArgv0(String argv0) {
private String escapeArgvRest(List<String> argv) {
if (windowsStyleArgEscaping) {
StringBuilder result = new StringBuilder();
boolean first = true;
for (String arg : argv) {
if (first) {
first = false;
} else {
result.append(" ");
}
result.append(ShellUtils.windowsEscapeArg(arg));
}
return result.toString();
} else {
return WindowsProcesses.quoteCommandLine(argv);
}
}

public static String processArgv0(String argv0) {
// Normalize the path and make it Windows-style.
// If argv0 is at least MAX_PATH (260 chars) long, createNativeProcess calls GetShortPathNameW
// to obtain a 8dot3 name for it (thereby support long paths in CreateProcessA), but then argv0
Expand All @@ -83,10 +101,10 @@ public String processArgv0(String argv0) {
// If it's not absolute, then it cannot be longer than MAX_PATH, since MAX_PATH also limits the
// length of file names.
PathFragment argv0fragment = PathFragment.create(argv0);
return (argv0fragment.isAbsolute()) ? argv0fragment.getPathString().replace('/', '\\') : argv0;
return argv0fragment.isAbsolute() ? argv0fragment.getPathString().replace('/', '\\') : argv0;
}

private String getRedirectPath(StreamAction action, File file) {
private static String getRedirectPath(StreamAction action, File file) {
switch (action) {
case DISCARD:
return "NUL"; // That's /dev/null on Windows
Expand All @@ -102,10 +120,8 @@ private String getRedirectPath(StreamAction action, File file) {
}
}

/**
* Converts an environment map to the format expected in lpEnvironment by CreateProcess().
*/
private byte[] convertEnvToNative(Map<String, String> envMap) throws IOException {
/** Converts an environment map to the format expected in lpEnvironment by CreateProcess(). */
private static byte[] convertEnvToNative(Map<String, String> envMap) throws IOException {
Map<String, String> realEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
Map<String, String> systemEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
if (envMap != null) {
Expand Down
26 changes: 13 additions & 13 deletions src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public boolean isLoggable(LogRecord record) {

private FileSystem setupEnvironmentForFakeExecution() {
// Prevent any subprocess execution at all.
SubprocessBuilder.setSubprocessFactory(new SubprocessInterceptor());
SubprocessBuilder.setDefaultSubprocessFactory(new SubprocessInterceptor());
resourceManager.setAvailableResources(
ResourceSet.create(/*memoryMb=*/1, /*cpuUsage=*/1, /*localTestCount=*/1));
return new InMemoryFileSystem();
Expand All @@ -292,7 +292,7 @@ private FileSystem setupEnvironmentForFakeExecution() {
*/
@Before
public final void setupEnvironmentForRealExecution() {
SubprocessBuilder.setSubprocessFactory(JavaSubprocessFactory.INSTANCE);
SubprocessBuilder.setDefaultSubprocessFactory(JavaSubprocessFactory.INSTANCE);
resourceManager.setAvailableResources(LocalHostCapacity.getLocalHostCapacity());
}

Expand All @@ -308,7 +308,7 @@ public void vanillaZeroExit() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 456;
Expand Down Expand Up @@ -364,7 +364,7 @@ public void testParamFiles() throws Exception {

SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 456;
Expand Down Expand Up @@ -414,7 +414,7 @@ public void noProcessWrapper() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 456;
Expand Down Expand Up @@ -460,7 +460,7 @@ public void nonZeroExit() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(3));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
LocalSpawnRunner runner =
Expand Down Expand Up @@ -508,7 +508,7 @@ public void processStartupThrows() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenThrow(new IOException("I'm sorry, Dave"));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
LocalSpawnRunner runner =
Expand Down Expand Up @@ -595,7 +595,7 @@ public void waitFor() throws InterruptedException {
}
}
});
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
LocalSpawnRunner runner =
Expand Down Expand Up @@ -627,7 +627,7 @@ public void checkPrefetchCalled() throws Exception {

SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
LocalSpawnRunner runner =
Expand All @@ -654,7 +654,7 @@ public void checkNoPrefetchCalled() throws Exception {

SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
LocalSpawnRunner runner =
Expand Down Expand Up @@ -684,7 +684,7 @@ public void checkLocalEnvProviderCalled() throws Exception {

SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);
LocalEnvProvider localEnvProvider = mock(LocalEnvProvider.class);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
Expand Down Expand Up @@ -723,7 +723,7 @@ public void useCorrectExtensionOnWindows() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 654;
Expand Down Expand Up @@ -973,7 +973,7 @@ public void relativePath() throws Exception {
SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
Expand Down
Loading

0 comments on commit 1da4861

Please sign in to comment.