From f2d459502f5fb422d6000db782795cffc6efa3e4 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Wed, 9 Aug 2017 15:27:49 +0200 Subject: 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 --- .../build/lib/exec/local/LocalSpawnRunnerTest.java | 7 +- .../build/lib/shell/CommandLargeInputsTest.java | 55 ++-- .../devtools/build/lib/shell/CommandTest.java | 278 ++++++--------------- .../devtools/build/lib/shell/ConsumersTest.java | 30 +-- .../build/lib/shell/InterruptibleTest.java | 33 ++- .../build/lib/shell/SimpleKillableObserver.java | 47 ++++ 6 files changed, 186 insertions(+), 264 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/shell/SimpleKillableObserver.java (limited to 'src/test') 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 @@ -134,6 +134,11 @@ public class LocalSpawnRunnerTest { new SpawnBuilder("/bin/echo", "Hi!").withEnvironment("VARIABLE", "value").build(); 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 env = Collections.singletonMap("foo", "bar"); - final String[] commandArgs = new String[] { "command" }; - final Command command = new Command(commandArgs, env, workingDir); + File workingDir = new File("."); + Map 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 env = Collections.singletonMap("FOO", "BAR"); - final Command command = new Command(new String[] {"/bin/sh", "-c", "echo $FOO"}, env, - null); + Map 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,90 +184,49 @@ 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(); tempFile.delete(); } - @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) { @@ -405,60 +345,20 @@ 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.

+ * + *

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(); + } + } +} -- cgit v1.2.3