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

8 files changed, 151 insertions, 106 deletions

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 2a8fa2ba47..f0414df5d2 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.runtime.proto.InvocationPolicyOuterClass.In
 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 @@ public final class BlazeRuntime {
     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 46eefeec6a..291dd32ce9 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 @@ public final class Command {
     Preconditions.checkNotNull(stdinInput, "stdinInput");
     logCommand();
 
-    final Subprocess process = startProcess();
+    Subprocess process = startProcess();
 
     outErrConsumers.logConsumptionStrategy();
     outErrConsumers.registerInputs(
@@ -369,22 +369,7 @@ public final class Command {
     // 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 @@ public final class Command {
     }
   }
 
-  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 0000000000..a8378a3343
--- /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 28464ba214..a7ebc8f7a1 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 @@ import java.util.concurrent.atomic.AtomicBoolean;
 /**
  * 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 3b85aafab5..f058a9ace9 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 @@ import java.io.OutputStream;
 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 ac21d64c8d..46d273fca2 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 @@ public class SubprocessBuilder {
   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 0000000000..f0b100124d
--- /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 e398a3b195..f022ae47ff 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.common.base.Charsets;
 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 @@ import java.util.TreeMap;
 /**
  * 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() {