Rewrite the Command API

Important: the simplified API now defaults to forwarding interrupts to
subprocesses. I did audit all the call sites, and I think this is a safe change
to make.

- Properly support timeouts with all implementations
- Simplify the API
  - only provide two flavours of blocking calls, which require no input and
    forward interrupts; this is the most common usage
  - provide a number of async calls, which optionally takes input, and a flag
    whether to forward interrupts
  - only support input streams, no byte arrays or other 'convenience features'
    that are rarely needed and unnecessarily increase the surface area
  - use java.time.Duration to specify timeout; for consistency, interpret a
    timeout of <= 0 as no timeout (i.e., including rather than excluding 0)
  - KillableObserver and subclasses are no longer part of the public API, but
    still used to implement timeouts if the Subprocess.Factory does not support
    them
- Update the documentation for Command
- Update all callers; most callers now use the simplified API

PiperOrigin-RevId: 164716782

6 files changed, 186 insertions, 264 deletions

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 7ec773d23f..74a4193498 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
@@ -135,6 +135,11 @@ public class LocalSpawnRunnerTest {
 
   private static final class SubprocessInterceptor implements Subprocess.Factory {
     @Override
+    public boolean supportsTimeout() {
+      return true;
+    }
+
+    @Override
     public Subprocess create(SubprocessBuilder params) throws IOException {
       throw new UnsupportedOperationException();
     }
@@ -261,7 +266,7 @@ public class LocalSpawnRunnerTest {
                 "/bin/echo",
                 "Hi!"));
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
-    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(-1);
+    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(0);
 
     assertThat(policy.lockOutputFilesCalled).isTrue();
     assertThat(policy.reportedStatus)
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandLargeInputsTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandLargeInputsTest.java
index f62e01b71d..9884be2adb 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/CommandLargeInputsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/CommandLargeInputsTest.java
@@ -57,54 +57,57 @@ public class CommandLargeInputsTest {
   }
 
   @Test
-  public void testCatRandomBinary() throws Exception {
+  public void testCatRandomBinaryToOutputStream() throws Exception {
     final Command command = new Command(new String[] {"cat"});
     byte[] randomBytes = getRandomBytes();
-    final CommandResult result = command.execute(randomBytes);
+    ByteArrayInputStream in = new ByteArrayInputStream(randomBytes);
+
+    CommandResult result =
+        command.executeAsync(in, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     assertThat(result.getTerminationStatus().getRawExitCode()).isEqualTo(0);
     TestUtil.assertArrayEquals(randomBytes, result.getStdout());
     assertThat(result.getStderr()).isEmpty();
    }
 
   @Test
-  public void testCatRandomBinaryToOutputStream() throws Exception {
-    final Command command = new Command(new String[] {"cat"});
-    ByteArrayOutputStream out = new ByteArrayOutputStream();
-    ByteArrayOutputStream err = new ByteArrayOutputStream();
+  public void testCatRandomBinaryToErrorStream() throws Exception {
+    final Command command = new Command(new String[] {"/bin/sh", "-c", "cat >&2"});
     byte[] randomBytes = getRandomBytes();
-    final CommandResult result = command.execute(randomBytes,
-                                                 Command.NO_OBSERVER, out, err);
+    ByteArrayInputStream in = new ByteArrayInputStream(randomBytes);
+
+    CommandResult result =
+        command.executeAsync(in, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     assertThat(result.getTerminationStatus().getRawExitCode()).isEqualTo(0);
-    TestUtil.assertArrayEquals(randomBytes, out.toByteArray());
-    assertThat(err.toByteArray()).isEmpty();
-    assertOutAndErrNotAvailable(result);
-  }
+    TestUtil.assertArrayEquals(randomBytes, result.getStderr());
+    assertThat(result.getStdout()).isEmpty();
+   }
 
   @Test
-  public void testCatRandomBinaryToErrorStream() throws Exception {
-    final Command command = new Command(new String[] {"/bin/sh", "-c", "cat >&2"});
+  public void testCatRandomBinaryFromInputStreamToOutputStream() throws Exception {
+    final Command command = new Command(new String[] {"cat"});
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     ByteArrayOutputStream err = new ByteArrayOutputStream();
     byte[] randomBytes = getRandomBytes();
-    final CommandResult result = command.execute(randomBytes,
-                                                 Command.NO_OBSERVER, out, err);
+    ByteArrayInputStream in = new ByteArrayInputStream(randomBytes);
+
+    CommandResult result =
+        command.executeAsync(in, out, err, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     assertThat(result.getTerminationStatus().getRawExitCode()).isEqualTo(0);
-    assertThat(out.toByteArray()).isEmpty();
-    TestUtil.assertArrayEquals(randomBytes, err.toByteArray());
+    assertThat(err.toByteArray()).isEmpty();
+    TestUtil.assertArrayEquals(randomBytes, out.toByteArray());
     assertOutAndErrNotAvailable(result);
   }
 
   @Test
-  public void testCatRandomBinaryFromInputStreamToErrorStream()
-  throws Exception {
+  public void testCatRandomBinaryFromInputStreamToErrorStream() throws Exception {
     final Command command = new Command(new String[] {"/bin/sh", "-c", "cat >&2"});
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     ByteArrayOutputStream err = new ByteArrayOutputStream();
     byte[] randomBytes = getRandomBytes();
     ByteArrayInputStream in = new ByteArrayInputStream(randomBytes);
 
-    final CommandResult result = command.execute(in,
-                                                 Command.NO_OBSERVER, out, err);
+    CommandResult result =
+        command.executeAsync(in, out, err, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     assertThat(result.getTerminationStatus().getRawExitCode()).isEqualTo(0);
     assertThat(out.toByteArray()).isEmpty();
     TestUtil.assertArrayEquals(randomBytes, err.toByteArray());
@@ -118,7 +121,7 @@ public class CommandLargeInputsTest {
     });
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     ByteArrayOutputStream err = new ByteArrayOutputStream();
-    command.execute(Command.NO_INPUT, Command.NO_OBSERVER, out, err);
+    command.execute(out, err);
     StringBuilder expectedOut = new StringBuilder();
     StringBuilder expectedErr = new StringBuilder();
     for (int i = 0; i < 1000; i++) {
@@ -144,10 +147,12 @@ public class CommandLargeInputsTest {
   public void testCatAllByteValues() throws Exception {
     final Command command = new Command(new String[] {"cat"});
     byte[] allByteValues = getAllByteValues();
-    final CommandResult result = command.execute(allByteValues);
+    ByteArrayInputStream in = new ByteArrayInputStream(allByteValues);
+
+    CommandResult result =
+        command.executeAsync(in, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     assertThat(result.getTerminationStatus().getRawExitCode()).isEqualTo(0);
     assertThat(result.getStderr()).isEmpty();
     TestUtil.assertArrayEquals(allByteValues, result.getStdout());
   }
-
 }
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java
index 45ba27b47a..a4aab8611a 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/CommandTest.java
@@ -27,6 +27,7 @@ import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.time.Duration;
 import java.util.Collections;
 import java.util.Map;
 import java.util.logging.Level;
@@ -37,8 +38,8 @@ import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /**
- * Unit tests for {@link Command}. This test will only succeed on Linux,
- * currently, because of its non-portable nature.
+ * Unit tests for {@link Command}. This test will only succeed on Linux, currently, because of its
+ * non-portable nature.
  */
 @RunWith(JUnit4.class)
 public class CommandTest {
@@ -50,18 +51,16 @@ public class CommandTest {
 
   @Before
   public final void configureLogger() throws Exception  {
-    // enable all log statements to ensure there are no problems with
-    // logging code
+    // Enable all log statements to ensure there are no problems with logging code.
     Logger.getLogger("com.google.devtools.build.lib.shell.Command").setLevel(Level.FINEST);
   }
 
   @Test
   public void testIllegalArgs() throws Exception {
-
     try {
       new Command((String[]) null);
-      fail("Should have thrown IllegalArgumentException");
-    } catch (IllegalArgumentException iae) {
+      fail("Should have thrown NullPointerException");
+    } catch (NullPointerException iae) {
       // good
     }
 
@@ -73,7 +72,8 @@ public class CommandTest {
     }
 
     try {
-      new Command(new String[] {"foo"}).execute(null);
+      Command r = new Command(new String[] {"foo"});
+      r.executeAsync((InputStream) null, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
       fail("Should have thrown NullPointerException");
     } catch (NullPointerException npe) {
       // good
@@ -83,12 +83,12 @@ public class CommandTest {
 
   @Test
   public void testGetters() {
-    final File workingDir = new File(".");
-    final Map<String,String> env = Collections.singletonMap("foo", "bar");
-    final String[] commandArgs = new String[] { "command" };
-    final Command command = new Command(commandArgs, env, workingDir);
+    File workingDir = new File(".");
+    Map<String, String> env = Collections.singletonMap("foo", "bar");
+    String[] commandArgs = new String[] { "command" };
+    Command command = new Command(commandArgs, env, workingDir);
     assertArrayEquals(commandArgs, command.getCommandLineElements());
-    for (final String key : env.keySet()) {
+    for (String key : env.keySet()) {
       assertThat(command.getEnvironmentVariables()).containsEntry(key, env.get(key));
     }
     assertThat(command.getWorkingDirectory()).isEqualTo(workingDir);
@@ -98,8 +98,8 @@ public class CommandTest {
 
   @Test
   public void testSimpleCommand() throws Exception {
-    final Command command = new Command(new String[] {"ls"});
-    final CommandResult result = command.execute();
+    Command command = new Command(new String[] {"ls"});
+    CommandResult result = command.execute();
     assertThat(result.getTerminationStatus().success()).isTrue();
     assertThat(result.getStderr()).isEmpty();
     assertThat(result.getStdout().length).isGreaterThan(0);
@@ -107,36 +107,36 @@ public class CommandTest {
 
   @Test
   public void testArguments() throws Exception {
-    final Command command = new Command(new String[] {"echo", "foo"});
+    Command command = new Command(new String[] {"echo", "foo"});
     checkSuccess(command.execute(), "foo\n");
   }
 
   @Test
   public void testEnvironment() throws Exception {
-    final Map<String,String> env = Collections.singletonMap("FOO", "BAR");
-    final Command command = new Command(new String[] {"/bin/sh", "-c", "echo $FOO"}, env,
-        null);
+    Map<String, String> env = Collections.singletonMap("FOO", "BAR");
+    Command command = new Command(new String[] {"/bin/sh", "-c", "echo $FOO"}, env, null);
     checkSuccess(command.execute(), "BAR\n");
   }
 
   @Test
   public void testWorkingDir() throws Exception {
-    final Command command = new Command(new String[] {"pwd"}, null, new File("/"));
+    Command command = new Command(new String[] {"pwd"}, null, new File("/"));
     checkSuccess(command.execute(), "/\n");
   }
 
   @Test
   public void testStdin() throws Exception {
-    final Command command = new Command(new String[] {"grep", "bar"});
-    checkSuccess(command.execute("foobarbaz".getBytes()), "foobarbaz\n");
+    Command command = new Command(new String[] {"grep", "bar"});
+    InputStream in = new ByteArrayInputStream("foobarbaz".getBytes());
+    checkSuccess(
+        command.executeAsync(in, Command.KILL_SUBPROCESS_ON_INTERRUPT).get(),
+        "foobarbaz\n");
   }
 
   @Test
   public void testRawCommand() throws Exception {
-    final Command command = new Command(new String[] { "perl",
-                                                       "-e",
-                                                       "print 'a'x100000" });
-    final CommandResult result = command.execute();
+    Command command = new Command(new String[] { "perl", "-e", "print 'a'x100000" });
+    CommandResult result = command.execute();
     assertThat(result.getTerminationStatus().success()).isTrue();
     assertThat(result.getStderr()).isEmpty();
     assertThat(result.getStdout().length).isGreaterThan(0);
@@ -144,49 +144,29 @@ public class CommandTest {
 
   @Test
   public void testRawCommandWithDir() throws Exception {
-    final Command command = new Command(new String[] { "pwd" },
-                                        null,
-                                        new File("/"));
-    final CommandResult result = command.execute();
+    Command command = new Command(new String[] { "pwd" }, null, new File("/"));
+    CommandResult result = command.execute();
     checkSuccess(result, "/\n");
   }
 
   @Test
   public void testHugeOutput() throws Exception {
-    final Command command = new Command(new String[] {"perl", "-e", "print 'a'x100000"});
-    final CommandResult result = command.execute();
+    Command command = new Command(new String[] {"perl", "-e", "print 'a'x100000"});
+    CommandResult result = command.execute();
     assertThat(result.getTerminationStatus().success()).isTrue();
     assertThat(result.getStderr()).isEmpty();
     assertThat(result.getStdout()).hasLength(100000);
   }
 
   @Test
-  public void testIgnoreOutput() throws Exception {
-    final Command command = new Command(new String[] {"perl", "-e", "print 'a'x100000"});
-    final CommandResult result = command.execute(Command.NO_INPUT, null, true);
-    assertThat(result.getTerminationStatus().success()).isTrue();
-    try {
-      result.getStdout();
-      fail("Should have thrown IllegalStateException");
-    } catch (IllegalStateException ise) {
-      // good
-    }
-    try {
-      result.getStderr();
-      fail("Should have thrown IllegalStateException");
-    } catch (IllegalStateException ise) {
-      // good
-    }
-  }
-
-  @Test
   public void testNoStreamingInputForCat() throws Exception {
-    final Command command = new Command(new String[]{"/bin/cat"});
+    Command command = new Command(new String[]{"/bin/cat"});
     ByteArrayInputStream emptyInput = new ByteArrayInputStream(new byte[0]);
     ByteArrayOutputStream out = new ByteArrayOutputStream();
     ByteArrayOutputStream err = new ByteArrayOutputStream();
-    CommandResult result = command.execute(emptyInput,
-                                           Command.NO_OBSERVER, out, err);
+    CommandResult result = command
+        .executeAsync(emptyInput, out, err, Command.KILL_SUBPROCESS_ON_INTERRUPT)
+        .get();
     assertThat(result.getTerminationStatus().success()).isTrue();
     assertThat(out.toString("UTF-8")).isEmpty();
     assertThat(err.toString("UTF-8")).isEmpty();
@@ -194,7 +174,7 @@ public class CommandTest {
 
   @Test
   public void testNoInputForCat() throws Exception {
-    final Command command = new Command(new String[]{"/bin/cat"});
+    Command command = new Command(new String[]{"/bin/cat"});
     CommandResult result = command.execute();
     assertThat(result.getTerminationStatus().success()).isTrue();
     assertThat(new String(result.getStdout(), "UTF-8")).isEmpty();
@@ -204,22 +184,20 @@ public class CommandTest {
   @Test
   public void testProvidedOutputStreamCapturesHelloWorld() throws Exception {
     String helloWorld = "Hello, world.";
-    final Command command = new Command(new String[]{"/bin/echo", helloWorld});
+    Command command = new Command(new String[]{"/bin/echo", helloWorld});
     ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
     ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
-    command.execute(Command.NO_INPUT, Command.NO_OBSERVER, stdOut, stdErr);
+    command.execute(stdOut, stdErr);
     assertThat(stdOut.toString("UTF-8")).isEqualTo(helloWorld + "\n");
     assertThat(stdErr.toByteArray()).isEmpty();
   }
 
   @Test
   public void testAsynchronous() throws Exception {
-    final File tempFile = File.createTempFile("googlecron-test", "tmp");
+    File tempFile = File.createTempFile("googlecron-test", "tmp");
     tempFile.delete();
-    final Command command = new Command(new String[] {"touch", tempFile.getAbsolutePath()});
-    // Shouldn't throw any exceptions:
-    FutureCommandResult result =
-        command.executeAsynchronously(Command.NO_INPUT);
+    Command command = new Command(new String[] {"touch", tempFile.getAbsolutePath()});
+    FutureCommandResult result = command.executeAsync();
     result.get();
     assertThat(tempFile.exists()).isTrue();
     assertThat(result.isDone()).isTrue();
@@ -227,67 +205,28 @@ public class CommandTest {
   }
 
   @Test
-  public void testAsynchronousWithKillable() throws Exception {
-    final Command command = new Command(new String[] {"sleep", "5"});
-    final SimpleKillableObserver observer = new SimpleKillableObserver();
-    FutureCommandResult result =
-        command.executeAsynchronously(Command.NO_INPUT, observer);
-    assertThat(result.isDone()).isFalse();
-    observer.kill();
-    try {
-      result.get();
-    } catch (AbnormalTerminationException e) {
-      // Expects, but does not insist on termination with a signal.
-    }
-    assertThat(result.isDone()).isTrue();
-  }
-
-  @Test
   public void testAsynchronousWithOutputStreams() throws Exception {
-
-    final String helloWorld = "Hello, world.";
-    final Command command = new Command(new String[]{"/bin/echo", helloWorld});
-    final ByteArrayInputStream emptyInput =
-      new ByteArrayInputStream(new byte[0]);
-    final ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
-    final ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
-    FutureCommandResult result = command.executeAsynchronously(emptyInput,
-        Command.NO_OBSERVER,
+    String helloWorld = "Hello, world.";
+    Command command = new Command(new String[]{"/bin/echo", helloWorld});
+    ByteArrayInputStream emptyInput = new ByteArrayInputStream(new byte[0]);
+    ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
+    ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
+    FutureCommandResult result = command.executeAsync(
+        emptyInput,
         stdOut,
-        stdErr);
+        stdErr,
+        /*killSubprocess=*/ false);
     result.get(); // Make sure the process actually finished
     assertThat(stdOut.toString("UTF-8")).isEqualTo(helloWorld + "\n");
     assertThat(stdErr.toByteArray()).isEmpty();
   }
 
   @Test
-  public void testSimpleKillableObserver() throws Exception {
-    final Command command = new Command(new String[] {"sleep", "5"});
-    final SimpleKillableObserver observer = new SimpleKillableObserver();
-    new Thread() {
-      @Override
-      public void run() {
-        try {
-          command.execute(Command.NO_INPUT, observer, true);
-          fail();
-        } catch (CommandException e) {
-          // Good.
-          checkCommandElements(e, "sleep", "5");
-        }
-      }
-    }.start();
-    // We're racing against the actual startup of the other command. Wait for 10ms so it can start.
-    Thread.sleep(10);
-    observer.kill();
-  }
-
-  @Test
   public void testTimeout() throws Exception {
-    // Sleep for 3 seconds,
-    final Command command = new Command(new String[] {"sleep", "3"});
+    // Sleep for 3 seconds, but timeout after 1 second.
+    Command command = new Command(new String[] {"sleep", "3"}, null, null, Duration.ofSeconds(1));
     try {
-      // but timeout after 1 second
-      command.execute(Command.NO_INPUT, 1000L, false);
+      command.execute();
       fail("Should have thrown AbnormalTerminationException");
     } catch (AbnormalTerminationException ate) {
       // good
@@ -298,13 +237,14 @@ public class CommandTest {
 
   @Test
   public void testTimeoutDoesntFire() throws Exception {
-    final Command command = new Command(new String[] {"cat"});
-    command.execute(new byte[]{'H', 'i', '!'}, 2000L, false);
+    Command command = new Command(new String[] {"cat"}, null, null, Duration.ofSeconds(2));
+    InputStream in = new ByteArrayInputStream(new byte[]{'H', 'i', '!'});
+    command.executeAsync(in, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
   }
 
   @Test
   public void testCommandDoesNotExist() throws Exception {
-    final Command command = new Command(new String[]{"thisisnotreal"});
+    Command command = new Command(new String[]{"thisisnotreal"});
     try {
       command.execute();
       fail();
@@ -329,7 +269,7 @@ public class CommandTest {
   public void testExitCodes() throws Exception {
     // 0 => success
     {
-      String args[] = { "/bin/sh", "-c", "exit 0" };
+      String[] args = { "/bin/sh", "-c", "exit 0" };
       CommandResult result = new Command(args).execute();
       TerminationStatus status = result.getTerminationStatus();
       assertThat(status.success()).isTrue();
@@ -341,7 +281,7 @@ public class CommandTest {
     // which map to signals).
     for (int exit : new int[] { 1, 2, 3, 127, 128, 192, 255 }) {
       try {
-        String args[] = { "/bin/sh", "-c", "exit " + exit };
+        String[] args = { "/bin/sh", "-c", "exit " + exit };
         new Command(args).execute();
         fail("Should have exited with status " + exit);
       } catch (BadExitStatusException e) {
@@ -359,7 +299,7 @@ public class CommandTest {
     for (int exit : new int[] { -1, -2, -3 }) {
       int expected = 256 + exit;
       try {
-        String args[] = { "/bin/bash", "-c", "exit " + exit };
+        String[] args = { "/bin/bash", "-c", "exit " + exit };
         new Command(args).execute();
         fail("Should have exited with status " + expected);
       } catch (BadExitStatusException e) {
@@ -384,7 +324,7 @@ public class CommandTest {
           + TestConstants.JAVATESTS_ROOT
           + "/com/google/devtools/build/lib/shell/killmyself";
       try {
-        String args[] = { killmyself, "" + signal };
+        String[] args = { killmyself, "" + signal };
         new Command(args).execute();
         fail("Expected signal " + signal);
       } catch (AbnormalTerminationException e) {
@@ -406,59 +346,19 @@ public class CommandTest {
   }
 
   @Test
-  public void testDestroy() throws Exception {
-
-    // Sleep for 10 seconds,
-    final Command command = new Command(new String[] {"sleep", "10"});
-
-    // but kill it after 1
-    final KillableObserver killer = new KillableObserver() {
-      @Override
-      public void startObserving(final Killable killable) {
-        final Thread t = new Thread() {
-          @Override
-          public void run() {
-            try {
-              Thread.sleep(1000L);
-            } catch (InterruptedException ie) {
-              // continue
-            }
-            killable.kill();
-          }
-        };
-        t.start();
-      }
-      @Override
-      public void stopObserving(final Killable killable) {
-        // do nothing
-      }
-    };
-
-    try {
-      command.execute(Command.NO_INPUT, killer, false);
-      fail("Should have thrown AbnormalTerminationException");
-    } catch (AbnormalTerminationException ate) {
-      // Good.
-      checkCommandElements(ate, "sleep", "10");
-      checkATE(ate);
-    }
-  }
-
-  @Test
   public void testOnlyReadsPartialInput() throws Exception {
     // -c == --bytes, but -c also works on Darwin.
     Command command = new Command(new String[] {"head", "-c", "500"});
     OutputStream out = new ByteArrayOutputStream();
     InputStream in = new InputStream() {
-
       @Override
       public int read() {
         return 0; // write an unbounded amount
       }
-
     };
 
-    CommandResult result = command.execute(in, Command.NO_OBSERVER, out, out);
+    CommandResult result =
+        command.executeAsync(in, out, out, Command.KILL_SUBPROCESS_ON_INTERRUPT).get();
     TerminationStatus status = result.getTerminationStatus();
     assertThat(status.success()).isTrue();
   }
@@ -468,11 +368,10 @@ public class CommandTest {
     final Command command = new Command(
         // On darwin, /bin/sh does not support -n for the echo builtin.
         new String[] {"/bin/bash", "-c", "echo -n Foo; sleep 0.1; echo Bar"});
-    // We run this command, passing in a special output stream
-    // that records when each flush() occurs.
-    // We test that a flush occurs after writing "Foo"
-    // and that another flush occurs after writing "Bar\n".
-    final boolean[] flushed = new boolean[8];
+    // We run this command, passing in a special output stream that records when each flush()
+    // occurs. We test that a flush occurs after writing "Foo" and that another flush occurs after
+    // writing "Bar\n".
+    boolean[] flushed = new boolean[8];
     OutputStream out = new OutputStream() {
       private int count = 0;
       @Override
@@ -484,7 +383,7 @@ public class CommandTest {
         flushed[count] = true;
       }
     };
-    command.execute(Command.NO_INPUT, Command.NO_OBSERVER, out, System.err);
+    command.execute(out, System.err);
     assertThat(flushed[0]).isFalse();
     assertThat(flushed[1]).isFalse(); // 'F'
     assertThat(flushed[2]).isFalse(); // 'o'
@@ -495,40 +394,6 @@ public class CommandTest {
     assertThat(flushed[7]).isTrue(); // '\n'
   }
 
-  // See also InterruptibleTest.
-  @Test
-  public void testInterrupt() throws Exception {
-
-    // Sleep for 10 seconds,
-    final Command command = new Command(new String[] {"sleep", "10"});
-    // Easy but hacky way to let this thread "return" a result to this method
-    final CommandResult[] resultContainer = new CommandResult[1];
-    final Thread commandThread = new Thread() {
-      @Override
-      public void run() {
-        try {
-          resultContainer[0] = command.execute();
-        } catch (CommandException ce) {
-          fail(ce.toString());
-        }
-      }
-    };
-    commandThread.start();
-
-    Thread.sleep(1000L);
-
-    // but interrupt it after 1
-    commandThread.interrupt();
-
-    // should continue to wait and exit normally
-    commandThread.join();
-
-    final CommandResult result = resultContainer[0];
-    assertThat(result.getTerminationStatus().success()).isTrue();
-    assertThat(result.getStderr()).isEmpty();
-    assertThat(result.getStdout()).isEmpty();
-  }
-
   @Test
   public void testOutputStreamThrowsException() throws Exception {
     OutputStream out = new OutputStream () {
@@ -539,18 +404,17 @@ public class CommandTest {
     };
     Command command = new Command(new String[] {"/bin/echo", "foo"});
     try {
-      command.execute(Command.NO_INPUT, Command.NO_OBSERVER, out, out);
+      command.execute(out, out);
       fail();
     } catch (AbnormalTerminationException e) {
       // Good.
       checkCommandElements(e, "/bin/echo", "foo");
-      assertThat(e).hasMessage("java.io.IOException");
+      assertThat(e).hasMessageThat().isEqualTo("java.io.IOException");
     }
   }
 
   @Test
-  public void testOutputStreamThrowsExceptionAndCommandFails()
-  throws Exception {
+  public void testOutputStreamThrowsExceptionAndCommandFails() throws Exception {
     OutputStream out = new OutputStream () {
       @Override
       public void write(int b) throws IOException {
@@ -559,7 +423,7 @@ public class CommandTest {
     };
     Command command = new Command(new String[] {"cat", "/dev/thisisnotreal"});
     try {
-      command.execute(Command.NO_INPUT, Command.NO_OBSERVER, out, out);
+      command.execute(out, out);
       fail();
     } catch (AbnormalTerminationException e) {
       checkCommandElements(e, "cat", "/dev/thisisnotreal");
diff --git a/src/test/java/com/google/devtools/build/lib/shell/ConsumersTest.java b/src/test/java/com/google/devtools/build/lib/shell/ConsumersTest.java
index 479a8b6e53..ebead695c3 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/ConsumersTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/ConsumersTest.java
@@ -20,7 +20,6 @@ import static org.junit.Assert.fail;
 import com.google.devtools.build.lib.shell.Consumers.OutErrConsumers;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -62,7 +61,7 @@ public class ConsumersTest {
       outErr.waitForCompletion();
       fail();
     } catch (IOException e) {
-      assertThat(e).hasMessage(SECRET_MESSAGE);
+      assertThat(e).hasMessageThat().isEqualTo(SECRET_MESSAGE);
     }
   }
 
@@ -115,7 +114,7 @@ public class ConsumersTest {
     } catch (IOException e) {
       fail();
     } catch (Error e) {
-      assertThat(e).hasMessage(SECRET_MESSAGE);
+      assertThat(e).hasMessageThat().isEqualTo(SECRET_MESSAGE);
     }
   }
 
@@ -140,30 +139,7 @@ public class ConsumersTest {
       outErr.waitForCompletion();
       fail();
     } catch (RuntimeException e) {
-      assertThat(e).hasMessage(SECRET_MESSAGE);
+      assertThat(e).hasMessageThat().isEqualTo(SECRET_MESSAGE);
     }
   }
-
-  /**
-   * Basically tests that Consumers#silentClose(InputStream) works properly.
-   */
-  @Test
-  public void testSilentlyDropIOExceptionWhenClosingInputStream() {
-    InputStream in = new ByteArrayInputStream(new byte[]{'a'}){
-      @Override
-      public void close() throws IOException {
-        throw new IOException("Please ignore me!");
-      }
-    };
-    OutErrConsumers outErr = Consumers.createDiscardingConsumers();
-    outErr.registerInputs(in, in, false);
-    try {
-      outErr.waitForCompletion();
-      // yeah!
-    } catch (IOException e) {
-      fail(); // this should not throw an exception, since we're silently
-              // closing the output!
-    }
-  }
-
 }
diff --git a/src/test/java/com/google/devtools/build/lib/shell/InterruptibleTest.java b/src/test/java/com/google/devtools/build/lib/shell/InterruptibleTest.java
index 006f89113d..20d70d8f17 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/InterruptibleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/InterruptibleTest.java
@@ -13,7 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.shell;
 
+import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static org.junit.Assert.fail;
 
 import org.junit.After;
 import org.junit.Before;
@@ -71,13 +73,16 @@ public class InterruptibleTest {
   }
 
   /**
-   * Test that interrupting a thread in an "uninterruptible" Command.execute
-   * preserves the thread's interruptible status, and does not terminate the
-   * subprocess.
+   * Test that interrupting a thread in an "uninterruptible" Command.execute marks the thread as
+   * interrupted, and does not terminate the subprocess.
    */
   @Test
   public void testUninterruptibleCommandRunsToCompletion() throws Exception {
-    command.execute();
+    CommandResult result =
+        command.executeAsync(Command.NO_INPUT, Command.CONTINUE_SUBPROCESS_ON_INTERRUPT).get();
+    assertThat(result.getTerminationStatus().success()).isTrue();
+    assertThat(result.getStderr()).isEmpty();
+    assertThat(result.getStdout()).isEmpty();
 
     // The interrupter thread should have exited about 1000ms ago.
     assertWithMessage("Interrupter thread is still alive!").that(interrupter.isAlive()).isFalse();
@@ -87,4 +92,24 @@ public class InterruptibleTest {
         .that(mainThread.isInterrupted())
         .isTrue();
   }
+
+  /**
+   * Test that interrupting a thread in an "interruptible" Command.execute does terminate the
+   * subprocess, and also marks the thread as interrupted.
+   */
+  @Test
+  public void testInterruptibleCommandRunsToCompletion() throws Exception {
+    try {
+      command.execute();
+      fail();
+    } catch (AbnormalTerminationException expected) {
+      assertThat(expected).hasMessageThat().isEqualTo("Process terminated by signal 15");
+      assertThat(expected.getResult().getTerminationStatus().exited()).isFalse();
+    }
+
+    // The interrupter thread should have set the main thread's interrupt flag.
+    assertWithMessage("Main thread was not interrupted during command execution!")
+        .that(mainThread.isInterrupted())
+        .isTrue();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java b/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java
new file mode 100644
index 0000000000..3a2e4a633a
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java
@@ -0,0 +1,47 @@
+// Copyright 2014 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;
+
+/**
+ * A simple implementation of {@link KillableObserver} which can be told explicitly to kill its
+ * {@link Killable} by calling {@link #kill()}. This is the sort of functionality that callers might
+ * expect to find available on the {@link Command} class.</p>
+ *
+ * <p>Note that this class can only observe one {@link Killable} at a time; multiple instances
+ * should be used for concurrent calls to {@link Command#execute}.
+ */
+final class SimpleKillableObserver implements KillableObserver {
+  private Killable killable;
+
+  @Override
+  public synchronized void startObserving(final Killable killable) {
+    this.killable = killable;
+  }
+
+  @Override
+  public synchronized void stopObserving(final Killable killable) {
+    if (!this.killable.equals(killable)) {
+      throw new IllegalStateException("start/stopObservering called with " +
+                                      "different Killables");
+    }
+    this.killable = null;
+  }
+
+  public synchronized void kill() {
+    if (killable != null) {
+      killable.kill();
+    }
+  }
+}