Move Subprocess.Factory to a top-level class
Also move the implementation of FutureCommandResult to a top-level class.
This is in preparation for significantly simplifying the shell library. The
plan is to remove the Subprocess abstraction, and have lower-level
implementations implement the much simpler FutureCommandResult interface
instead.
PiperOrigin-RevId: 167844736
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 2a8fa2b..f0414df 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
@@ -54,8 +54,8 @@
import com.google.devtools.build.lib.server.RPCServer;
import com.google.devtools.build.lib.server.signal.InterruptSignalHandler;
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.shell.SubprocessFactory;
import com.google.devtools.build.lib.unix.UnixFileSystem;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.CustomExitCodePublisher;
@@ -817,7 +817,7 @@
return OS.getCurrent() == OS.WINDOWS ? new WindowsFileSystem() : new UnixFileSystem();
}
- private static Subprocess.Factory subprocessFactoryImplementation() {
+ private static SubprocessFactory subprocessFactoryImplementation() {
if (!"0".equals(System.getProperty("io.bazel.EnableJni")) && OS.getCurrent() == OS.WINDOWS) {
return WindowsSubprocessFactory.INSTANCE;
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Command.java b/src/main/java/com/google/devtools/build/lib/shell/Command.java
index 46eefee..291dd32 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/Command.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/Command.java
@@ -358,7 +358,7 @@
Preconditions.checkNotNull(stdinInput, "stdinInput");
logCommand();
- final Subprocess process = startProcess();
+ Subprocess process = startProcess();
outErrConsumers.logConsumptionStrategy();
outErrConsumers.registerInputs(
@@ -369,22 +369,7 @@
// enforced.
processInput(stdinInput, process);
- return new FutureCommandResult() {
- @Override
- public CommandResult get() throws AbnormalTerminationException {
- return waitForProcessToComplete(process, outErrConsumers, killSubprocessOnInterrupt);
- }
-
- @Override
- public boolean isDone() {
- return process.finished();
- }
-
- @Override
- public void cancel() {
- process.destroy();
- }
- };
+ return new FutureCommandResultImpl(this, process, outErrConsumers, killSubprocessOnInterrupt);
}
private Subprocess startProcess() throws ExecFailedException {
@@ -425,78 +410,6 @@
}
}
- private CommandResult waitForProcessToComplete(
- Subprocess process, Consumers.OutErrConsumers outErr, boolean killSubprocessOnInterrupt)
- throws AbnormalTerminationException {
- logger.finer("Waiting for process...");
-
- TerminationStatus status = waitForProcess(process, killSubprocessOnInterrupt);
-
- logger.finer(status.toString());
-
- try {
- if (Thread.currentThread().isInterrupted()) {
- outErr.cancel();
- } else {
- outErr.waitForCompletion();
- }
- } catch (IOException ioe) {
- CommandResult noOutputResult =
- new CommandResult(CommandResult.EMPTY_OUTPUT,
- CommandResult.EMPTY_OUTPUT,
- status);
- if (status.success()) {
- // If command was otherwise successful, throw an exception about this
- throw new AbnormalTerminationException(this, noOutputResult, ioe);
- } else {
- // Otherwise, throw the more important exception -- command
- // was not successful
- String message = status
- + "; also encountered an error while attempting to retrieve output";
- throw status.exited()
- ? new BadExitStatusException(this, noOutputResult, message, ioe)
- : new AbnormalTerminationException(this,
- noOutputResult, message, ioe);
- }
- } finally {
- process.close();
- }
-
- CommandResult result =
- new CommandResult(outErr.getAccumulatedOut(), outErr.getAccumulatedErr(), status);
- result.logThis();
- if (status.success()) {
- return result;
- } else if (status.exited()) {
- throw new BadExitStatusException(this, result, status.toString());
- } else {
- throw new AbnormalTerminationException(this, result, status.toString());
- }
- }
-
- private static TerminationStatus waitForProcess(
- Subprocess process, boolean killSubprocessOnInterrupt) {
- boolean wasInterrupted = false;
- try {
- while (true) {
- try {
- process.waitFor();
- return new TerminationStatus(process.exitValue(), process.timedout());
- } catch (InterruptedException ie) {
- wasInterrupted = true;
- if (killSubprocessOnInterrupt) {
- process.destroy();
- }
- }
- }
- } finally {
- // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/
- if (wasInterrupted) {
- Thread.currentThread().interrupt(); // preserve interrupted status
- }
- }
- }
-
private void logCommand() {
if (!logger.isLoggable(Level.FINE)) {
return;
diff --git a/src/main/java/com/google/devtools/build/lib/shell/FutureCommandResultImpl.java b/src/main/java/com/google/devtools/build/lib/shell/FutureCommandResultImpl.java
new file mode 100644
index 0000000..a8378a3
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/shell/FutureCommandResultImpl.java
@@ -0,0 +1,114 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.shell;
+
+import com.google.devtools.build.lib.shell.Consumers.OutErrConsumers;
+import java.io.IOException;
+
+/**
+ * Basic and only implementation of {@link FutureCommandResult} for use by implementations of
+ * {@link SubprocessFactory}.
+ */
+final class FutureCommandResultImpl implements FutureCommandResult {
+ private final Command command;
+ private final Subprocess process;
+ private final OutErrConsumers outErrConsumers;
+ private final boolean killSubprocessOnInterrupt;
+
+ public FutureCommandResultImpl(
+ Command command,
+ Subprocess process,
+ OutErrConsumers outErrConsumers,
+ boolean killSubprocessOnInterrupt) {
+ this.command = command;
+ this.process = process;
+ this.outErrConsumers = outErrConsumers;
+ this.killSubprocessOnInterrupt = killSubprocessOnInterrupt;
+ }
+
+ @Override
+ public CommandResult get() throws AbnormalTerminationException {
+ TerminationStatus status = waitForProcess(process, killSubprocessOnInterrupt);
+ try {
+ if (Thread.currentThread().isInterrupted()) {
+ outErrConsumers.cancel();
+ } else {
+ outErrConsumers.waitForCompletion();
+ }
+ } catch (IOException ioe) {
+ CommandResult noOutputResult =
+ new CommandResult(CommandResult.EMPTY_OUTPUT,
+ CommandResult.EMPTY_OUTPUT,
+ status);
+ if (status.success()) {
+ // If command was otherwise successful, throw an exception about this
+ throw new AbnormalTerminationException(command, noOutputResult, ioe);
+ } else {
+ // Otherwise, throw the more important exception -- command
+ // was not successful
+ String message = status
+ + "; also encountered an error while attempting to retrieve output";
+ throw status.exited()
+ ? new BadExitStatusException(command, noOutputResult, message, ioe)
+ : new AbnormalTerminationException(command, noOutputResult, message, ioe);
+ }
+ } finally {
+ process.close();
+ }
+
+ CommandResult result = new CommandResult(
+ outErrConsumers.getAccumulatedOut(), outErrConsumers.getAccumulatedErr(), status);
+ result.logThis();
+ if (status.success()) {
+ return result;
+ } else if (status.exited()) {
+ throw new BadExitStatusException(command, result, status.toString());
+ } else {
+ throw new AbnormalTerminationException(command, result, status.toString());
+ }
+ }
+
+ private static TerminationStatus waitForProcess(
+ Subprocess process, boolean killSubprocessOnInterrupt) {
+ boolean wasInterrupted = false;
+ try {
+ while (true) {
+ try {
+ process.waitFor();
+ return new TerminationStatus(process.exitValue(), process.timedout());
+ } catch (InterruptedException ie) {
+ wasInterrupted = true;
+ if (killSubprocessOnInterrupt) {
+ process.destroy();
+ }
+ }
+ }
+ } finally {
+ // Read this for detailed explanation: http://www.ibm.com/developerworks/library/j-jtp05236/
+ if (wasInterrupted) {
+ Thread.currentThread().interrupt(); // preserve interrupted status
+ }
+ }
+ }
+
+ @Override
+ public void cancel() {
+ process.destroy();
+ }
+
+ @Override
+ public boolean isDone() {
+ return process.finished();
+ }
+}
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 28464ba..a7ebc8f 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
@@ -26,7 +26,7 @@
/**
* A subprocess factory that uses {@link java.lang.ProcessBuilder}.
*/
-public class JavaSubprocessFactory implements Subprocess.Factory {
+public class JavaSubprocessFactory implements SubprocessFactory {
/**
* A subprocess backed by a {@link java.lang.Process}.
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java b/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java
index 3b85aaf..f058a9a 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/Subprocess.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.shell;
import java.io.Closeable;
-import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -25,16 +24,6 @@
public interface Subprocess extends Closeable {
/**
- * Something that can create subprocesses.
- */
- interface Factory {
- /**
- * Create a subprocess according to the specified parameters.
- */
- Subprocess create(SubprocessBuilder params) throws IOException;
- }
-
- /**
* Kill the process.
*/
boolean destroy();
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 ac21d64..46d273f 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
@@ -49,9 +49,9 @@
private File workingDirectory;
private long timeoutMillis;
- static Subprocess.Factory factory = JavaSubprocessFactory.INSTANCE;
+ static SubprocessFactory factory = JavaSubprocessFactory.INSTANCE;
- public static void setSubprocessFactory(Subprocess.Factory factory) {
+ public static void setSubprocessFactory(SubprocessFactory factory) {
SubprocessBuilder.factory = factory;
}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/SubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/SubprocessFactory.java
new file mode 100644
index 0000000..f0b1001
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/shell/SubprocessFactory.java
@@ -0,0 +1,28 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.shell;
+
+import java.io.IOException;
+
+/**
+ * Something that can create subprocesses.
+ */
+public interface SubprocessFactory {
+ /**
+ * Create a subprocess according to the specified parameters.
+ *
+ * @throws IOException if the underlying file is not executable or cannot be found
+ */
+ Subprocess create(SubprocessBuilder params) throws IOException;
+}
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
index e398a3b..f022ae4 100644
--- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
@@ -18,6 +18,7 @@
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.jni.WindowsProcesses;
import java.io.File;
@@ -29,7 +30,7 @@
/**
* A subprocess factory that uses the Win32 API.
*/
-public class WindowsSubprocessFactory implements Subprocess.Factory {
+public class WindowsSubprocessFactory implements SubprocessFactory {
public static final WindowsSubprocessFactory INSTANCE = new WindowsSubprocessFactory();
private WindowsSubprocessFactory() {
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 4f187f7..fe84224 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -38,6 +38,7 @@
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.shell.SubprocessFactory;
import com.google.devtools.build.lib.util.NetUtil;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.io.FileOutErr;
@@ -131,7 +132,7 @@
private static final Spawn SIMPLE_SPAWN =
new SpawnBuilder("/bin/echo", "Hi!").withEnvironment("VARIABLE", "value").build();
- private static final class SubprocessInterceptor implements Subprocess.Factory {
+ private static final class SubprocessInterceptor implements SubprocessFactory {
@Override
public Subprocess create(SubprocessBuilder params) throws IOException {
throw new UnsupportedOperationException();
@@ -239,7 +240,7 @@
// T:\execroot\execroot\_bin\process-wrapper
return;
}
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
@@ -285,7 +286,7 @@
// T:\execroot\bin\echo
return;
}
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
@@ -322,7 +323,7 @@
// T:\execroot\execroot\_bin\process-wrapper
return;
}
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(3));
SubprocessBuilder.setSubprocessFactory(factory);
@@ -358,7 +359,7 @@
@Test
public void processStartupThrows() throws Exception {
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ 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);
@@ -406,7 +407,7 @@
@Test
public void interruptedException() throws Exception {
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(3) {
private boolean destroyed;
@@ -444,7 +445,7 @@
@Test
public void checkPrefetchCalled() throws Exception {
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
@@ -461,7 +462,7 @@
@Test
public void checkNoPrefetchCalled() throws Exception {
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
@@ -481,7 +482,7 @@
@Test
public void checkLocalEnvProviderCalled() throws Exception {
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
when(factory.create(any())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
LocalEnvProvider localEnvProvider = mock(LocalEnvProvider.class);
@@ -507,7 +508,7 @@
// T:\execroot\execroot\_bin\process-wrapper.exe
return;
}
- Subprocess.Factory factory = mock(Subprocess.Factory.class);
+ SubprocessFactory factory = mock(SubprocessFactory.class);
ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
SubprocessBuilder.setSubprocessFactory(factory);
