diff --git a/core/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/core/ProcessRunner.java b/core/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/core/ProcessRunner.java index b0491330f59..6a6694f3daf 100644 --- a/core/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/core/ProcessRunner.java +++ b/core/spring-boot-docker-compose/src/main/java/org/springframework/boot/docker/compose/core/ProcessRunner.java @@ -124,6 +124,7 @@ class ProcessRunner { } catch (InterruptedException ex) { Thread.currentThread().interrupt(); + process.destroy(); throw new IllegalStateException("Interrupted waiting for %s".formatted(process), ex); } } @@ -162,11 +163,13 @@ class ProcessRunner { } line = reader.readLine(); } - this.latch.countDown(); } catch (IOException ex) { throw new UncheckedIOException("Failed to read process stream", ex); } + finally { + this.latch.countDown(); + } } @Override diff --git a/core/spring-boot-docker-compose/src/test/java/org/springframework/boot/docker/compose/core/ProcessRunnerTests.java b/core/spring-boot-docker-compose/src/test/java/org/springframework/boot/docker/compose/core/ProcessRunnerTests.java index 5fc96511bd3..370bb88551c 100644 --- a/core/spring-boot-docker-compose/src/test/java/org/springframework/boot/docker/compose/core/ProcessRunnerTests.java +++ b/core/spring-boot-docker-compose/src/test/java/org/springframework/boot/docker/compose/core/ProcessRunnerTests.java @@ -16,7 +16,17 @@ package org.springframework.boot.docker.compose.core; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.time.Duration; +import java.util.concurrent.atomic.AtomicReference; + +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; import org.springframework.boot.testsupport.process.DisabledIfProcessUnavailable; @@ -29,24 +39,11 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; * @author Moritz Halbritter * @author Andy Wilkinson * @author Phillip Webb + * @author Sebastien Tardif */ -@DisabledIfProcessUnavailable("docker") class ProcessRunnerTests { - private ProcessRunner processRunner = new ProcessRunner(); - - @Test - void run() { - String out = this.processRunner.run("docker", "--version"); - assertThat(out).isNotEmpty(); - } - - @Test - void runWhenHasOutputConsumer() { - StringBuilder output = new StringBuilder(); - this.processRunner.run(output::append, "docker", "--version"); - assertThat(output.toString()).isNotEmpty(); - } + private final ProcessRunner processRunner = new ProcessRunner(); @Test void runWhenProcessDoesNotStart() { @@ -55,14 +52,69 @@ class ProcessRunnerTests { } @Test - void runWhenProcessReturnsNonZeroExitCode() { - assertThatExceptionOfType(ProcessExitException.class) - .isThrownBy(() -> this.processRunner.run("docker", "-thisdoesntwork")) - .satisfies((ex) -> { - assertThat(ex.getExitCode()).isGreaterThan(0); - assertThat(ex.getStdOut()).isEmpty(); - assertThat(ex.getStdErr()).isNotEmpty(); - }); + @DisabledOnOs(OS.WINDOWS) + void runWhenOutputConsumerThrowsDoesNotHang() throws InterruptedException { + Thread runner = new Thread(() -> this.processRunner.run((line) -> { + throw new IllegalStateException("boom"); + }, "echo", "hello"), "process-runner-consumer-throw-test"); + runner.start(); + runner.join(Duration.ofSeconds(5).toMillis()); + assertThat(runner.isAlive()).isFalse(); + } + + @Test + @DisabledOnOs(OS.WINDOWS) + void runWhenInterruptedDestroysChildProcess() throws Exception { + Path pidFile = Files.createTempFile("process-runner-", ".pid"); + Files.delete(pidFile); + AtomicReference error = new AtomicReference<>(); + Thread runner = new Thread(() -> { + try { + this.processRunner.run("sh", "-c", "echo $$ > '" + pidFile + "'; exec sleep 60"); + } + catch (Throwable ex) { + error.set(ex); + } + }, "process-runner-interrupt-test"); + runner.start(); + Awaitility.await().until(() -> Files.exists(pidFile, LinkOption.NOFOLLOW_LINKS)); + long pid = Long.parseLong(Files.readString(pidFile).trim()); + assertThat(ProcessHandle.of(pid)).isPresent().get().matches(ProcessHandle::isAlive); + runner.interrupt(); + runner.join(Duration.ofSeconds(5).toMillis()); + assertThat(runner.isAlive()).isFalse(); + assertThat(error.get()).isInstanceOf(IllegalStateException.class); + assertThat(ProcessHandle.of(pid).map(ProcessHandle::isAlive).orElse(false)).isFalse(); + } + + @Nested + @DisabledIfProcessUnavailable("docker") + class WhenDockerIsAvailable { + + @Test + void run() { + String out = ProcessRunnerTests.this.processRunner.run("docker", "--version"); + assertThat(out).isNotEmpty(); + } + + @Test + void runWhenHasOutputConsumer() { + StringBuilder output = new StringBuilder(); + ProcessRunnerTests.this.processRunner.run(output::append, "docker", "--version"); + assertThat(output.toString()).isNotEmpty(); + } + + @Test + void runWhenProcessReturnsNonZeroExitCode() { + assertThatExceptionOfType(ProcessExitException.class) + .isThrownBy(() -> ProcessRunnerTests.this.processRunner.run("docker", "-thisdoesntwork")) + .satisfies((ex) -> { + assertThat(ex.getExitCode()).isGreaterThan(0); + assertThat(ex.getStdOut()).isEmpty(); + assertThat(ex.getStdErr()).isNotEmpty(); + }); + } + } }