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

1 files changed, 71 insertions, 207 deletions

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");