diff --git a/src/main/java/com/testingbot/tunnel/App.java b/src/main/java/com/testingbot/tunnel/App.java index f9ebe3b..c4eed8e 100644 --- a/src/main/java/com/testingbot/tunnel/App.java +++ b/src/main/java/com/testingbot/tunnel/App.java @@ -463,14 +463,21 @@ public static void main(String... args) throws Exception { app.init(); app.boot(); + // The pid file lets an external supervisor stop this process; it is + // only meaningful when running as a command line client. + app.trackPid(); } catch (ParseException parseException) { System.err.println(parseException.getMessage()); System.exit(2); + } catch (TunnelFailedException tunnelFailedException) { + System.err.println(tunnelFailedException.getMessage()); + System.exit(tunnelFailedException.getExitCode()); } } private PidPoller pidPoller; private TunnelPoller poller; private HttpForwarder httpForwarder; + private Thread cleanupThread; private String[] getUserData() { if (System.getenv("TESTINGBOT_KEY") != null && System.getenv("TESTINGBOT_SECRET") != null) { @@ -492,7 +499,7 @@ private String[] getUserData() { } public void init() { - Thread cleanupThread = new Thread() { + cleanupThread = new Thread() { @Override public void run() { if (readyFile != null) { @@ -518,28 +525,28 @@ public void run() { Runtime.getRuntime().addShutdownHook(cleanupThread); } + Api createApi() { + return new Api(this); + } + public void boot() throws Exception { - api = new Api(this); + api = createApi(); JsonNode tunnelData = null; try { tunnelData = api.createTunnel(); } catch (Exception e) { - System.err.println("Creating a new tunnel failed, please make sure you're supplying correct credentials and that you can connect to the TestingBot network.\nUse --doctor to verify if everything is set up correctly."); - System.err.println(e.getMessage()); - System.exit(1); + throw new TunnelFailedException("Creating a new tunnel failed, please make sure you're supplying correct credentials and that you can connect to the TestingBot network.\nUse --doctor to verify if everything is set up correctly.\n" + e.getMessage(), 1, e); } if (tunnelData.has("error")) { - System.err.println("An error ocurred: " + tunnelData.get("error").asText()); + String error = "An error ocurred: " + tunnelData.get("error").asText(); if (tunnelData.get("error").asText().contains("401")) { - System.err.println("Missing required arguments API_KEY API_SECRET\nYou can get these two values from https://testingbot.com/members/user/edit"); + error += "\nMissing required arguments API_KEY API_SECRET\nYou can get these two values from https://testingbot.com/members/user/edit"; } - System.exit(1); + throw new TunnelFailedException(error, 1); } - trackPid(); - startInsightServer(); if (tunnelData.has("id")) { @@ -585,11 +592,31 @@ public void stop() { poller.cancel(); } - try { - System.out.println("Shutting down your personal Tunnel Server."); - api.destroyTunnel(); - } catch (Exception ex) { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); + if (pidPoller != null) { + pidPoller.cancel(); + pidPoller = null; + } + + // Without this, an embedder that starts a tunnel per job leaks one + // shutdown hook per App instance for the lifetime of the JVM. + if (cleanupThread != null) { + try { + Runtime.getRuntime().removeShutdownHook(cleanupThread); + } catch (IllegalStateException alreadyShuttingDown) { + // the JVM is on its way down and will run the hook itself + } + cleanupThread = null; + } + + // api is null when stop() is called after boot() failed, which is the + // normal path for an embedder cleaning up in a finally block. + if (api != null) { + try { + System.out.println("Shutting down your personal Tunnel Server."); + api.destroyTunnel(); + } catch (Exception ex) { + Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); + } } } @@ -607,6 +634,10 @@ public void tunnelReady(JsonNode apiResponse) { Logger.getLogger(App.class.getName()).log(Level.INFO, "The Tunnel is ready, ip: {0}\nYou may start your tests.", _serverIP); Logger.getLogger(App.class.getName()).log(Level.INFO, "To stop the tunnel, press CTRL+C"); } + } catch (TunnelFailedException tunnelFailedException) { + // fatal: let it reach the caller of boot() so the command line + // client can exit and an embedder can handle it + throw tunnelFailedException; } catch (Exception ex) { Logger.getLogger(App.class.getName()).log(Level.INFO, "Something went wrong while setting up the Tunnel."); Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); @@ -624,8 +655,7 @@ private void startProxies() { try { this.httpProxy = new HttpProxy(this); } catch (HttpProxy.HttpProxyStartException ex) { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, ex.getMessage()); - System.exit(1); + throw new TunnelFailedException(ex.getMessage(), 1, ex); } if (this.getProxy() == null && !this.httpProxy.testProxy()) { Logger.getLogger(App.class.getName()).log(Level.INFO, "! Tunnel might not work properly, test failed"); @@ -650,7 +680,7 @@ private void startProxies() { public void doctor() { Doctor doctor = new Doctor(this); if (doctor.hasFailures()) { - System.exit(1); + throw new TunnelFailedException("Doctor detected one or more problems, see the output above.", 1); } } diff --git a/src/main/java/com/testingbot/tunnel/TunnelFailedException.java b/src/main/java/com/testingbot/tunnel/TunnelFailedException.java new file mode 100644 index 0000000..34afb92 --- /dev/null +++ b/src/main/java/com/testingbot/tunnel/TunnelFailedException.java @@ -0,0 +1,30 @@ +package com.testingbot.tunnel; + +/** + * Thrown when the tunnel cannot be started or has to be aborted. + * + * Carries the exit code the command line client should terminate with, so that + * {@link App#main(String...)} keeps its existing exit codes while embedders can + * catch this instead of having their JVM terminated. + */ +public class TunnelFailedException extends RuntimeException { + private final int exitCode; + + public TunnelFailedException(String message) { + this(message, 1); + } + + public TunnelFailedException(String message, int exitCode) { + super(message); + this.exitCode = exitCode; + } + + public TunnelFailedException(String message, int exitCode, Throwable cause) { + super(message, cause); + this.exitCode = exitCode; + } + + public int getExitCode() { + return exitCode; + } +} diff --git a/src/main/java/ssh/TunnelPoller.java b/src/main/java/ssh/TunnelPoller.java index 6ded544..44cdc51 100644 --- a/src/main/java/ssh/TunnelPoller.java +++ b/src/main/java/ssh/TunnelPoller.java @@ -2,6 +2,7 @@ import com.testingbot.tunnel.Api; import com.testingbot.tunnel.App; +import com.testingbot.tunnel.TunnelFailedException; import java.util.Timer; import java.util.TimerTask; import java.util.logging.Level; @@ -51,6 +52,11 @@ public void run() { this.counter += 1; Logger.getLogger(TunnelPoller.class.getName()).log(Level.INFO, "Current tunnel status: {0}", response.get("state").asText()); } + } catch (TunnelFailedException tunnelFailedException) { + // the tunnel became ready but could not be set up; this runs on a + // timer thread so there is nobody to propagate to, report it here + timer.cancel(); + Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, tunnelFailedException.getMessage()); } catch (Exception ex) { timer.cancel(); Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, "Unable to poll for tunnel status."); diff --git a/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java b/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java new file mode 100644 index 0000000..ef894b4 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java @@ -0,0 +1,128 @@ +package com.testingbot.tunnel; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.client.WireMock; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Field; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Verifies that App can be embedded in a host process: a failing tunnel must + * report the failure to the caller instead of terminating the JVM. + * + * These tests would not merely fail but crash the surefire JVM if App went back + * to calling System.exit on the boot path. + */ +class AppEmbeddedTest { + + private WireMockServer wireMockServer; + private App app; + + @BeforeEach + void setUp() { + wireMockServer = new WireMockServer(options().dynamicPort()); + wireMockServer.start(); + WireMock.configureFor("localhost", wireMockServer.port()); + + app = new App() { + @Override + Api createApi() { + Api api = new Api(this); + api.setApiScheme("http"); + api.setApiHost("localhost:" + wireMockServer.port()); + return api; + } + }; + app.setClientKey("test_key"); + app.setClientSecret("test_secret"); + } + + @AfterEach + void tearDown() { + if (wireMockServer != null) { + wireMockServer.stop(); + } + } + + @Test + void boot_whenCredentialsAreRejected_shouldThrowInsteadOfExiting() { + // Given: the API reports an authentication failure + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"401 Unauthorized\"}"))); + + // When & Then: the caller gets an exception, the JVM survives + assertThatThrownBy(() -> app.boot()) + .isInstanceOf(TunnelFailedException.class) + .hasMessageContaining("401"); + } + + @Test + void boot_whenTunnelCreationFails_shouldThrowInsteadOfExiting() { + // Given: the API is unreachable / returns something unusable + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse().withStatus(500).withBody("nope"))); + + // When & Then + assertThatThrownBy(() -> app.boot()) + .isInstanceOf(TunnelFailedException.class); + } + + @Test + void boot_shouldNotStartPidPolling() throws Exception { + // Given: pid tracking is a command line concern; a background timer that + // calls System.exit has no business running inside a host process + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"401 Unauthorized\"}"))); + + // When + assertThatThrownBy(() -> app.boot()).isInstanceOf(TunnelFailedException.class); + + // Then + assertThat(readField(app, "pidPoller")).isNull(); + } + + @Test + void stop_shouldUnregisterTheShutdownHook() throws Exception { + // Given + app.init(); + assertThat(readField(app, "cleanupThread")).isNotNull(); + + // When + app.stop(); + + // Then: an embedder starting a tunnel per job must not leak a hook each time + assertThat(readField(app, "cleanupThread")).isNull(); + } + + @Test + void stop_shouldBeSafeToCallRepeatedly() { + // Given + app.init(); + + // When & Then + assertThatCode(() -> { + app.stop(); + app.stop(); + }).doesNotThrowAnyException(); + } + + private Object readField(App target, String name) throws Exception { + Field field = App.class.getDeclaredField(name); + field.setAccessible(true); + return field.get(target); + } +} diff --git a/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java b/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java new file mode 100644 index 0000000..3c54f00 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java @@ -0,0 +1,40 @@ +package com.testingbot.tunnel; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +class TunnelFailedExceptionTest { + + @Test + void defaultExitCode_shouldBeOne() { + // Given & When + TunnelFailedException exception = new TunnelFailedException("boom"); + + // Then + assertThat(exception.getMessage()).isEqualTo("boom"); + assertThat(exception.getExitCode()).isEqualTo(1); + } + + @Test + void exitCode_shouldBePreserved() { + // Given & When + TunnelFailedException exception = new TunnelFailedException("boom", 2); + + // Then + assertThat(exception.getExitCode()).isEqualTo(2); + } + + @Test + void cause_shouldBePreserved() { + // Given + Exception cause = new IllegalStateException("underlying"); + + // When + TunnelFailedException exception = new TunnelFailedException("boom", 1, cause); + + // Then + assertThat(exception).hasCause(cause); + assertThat(exception.getExitCode()).isEqualTo(1); + } +}