Don't call System.exit() when the tunnel is embedded

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

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;
+    }
+}

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

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);
+    }
+}

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);
+    }
+}