Repository: tipsy/javalin
Pull request #1441 (2021-11-28 18:53:32)

Enhance Javalin to support try with resources
Hi Tipsy,

First, thank you so much for creating and maintaining this wonderful project.

This PR simply makes Javalin implement AutoClosable so that it's compatible with try-with-resources. See tests for examples. Please let me know your thoughts and about any improvements I can make. Did my best to follow existing code style.

Thanks.

tipsy: Thank you @signedbit ! The code looks fine (tests too!), but could you elaborate on why you want this? What problem does this solve?
signedbit: Hi @tipsy. In short, try-with-resources allows all exceptions to be caught correctly without prior exceptions magically disappearing and ensures the client can't forget to close the resource. [See official docs for more](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html). It is very helpful for me because my application has many resources that need to be gracefully cleaned up, Javalin being one. It also allows for an elegant idiom for integration/e2e tests, particularly helpful for running tests concurrently:

```java
try (final Javalin app = Javalin.create().start(0)) {
    // ...
}
```

tipsy: @signedbit I have to admit I am intrigued, but it doesn't seem quite right. If you look at the other classes implementing `AutoCloseable`, they are mostly streams, channels, sockets. Not full servers. It is sort of elegant though, in particular for tests. I can't really think of any downsides to doing this, can you? 

Patch:
diff --git a/javalin/src/main/java/io/javalin/Javalin.java b/javalin/src/main/java/io/javalin/Javalin.java
index a8a4157f..6dc01ad7 100644
--- a/javalin/src/main/java/io/javalin/Javalin.java
+++ b/javalin/src/main/java/io/javalin/Javalin.java
@@ -40,11 +40,13 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.function.Consumer;
+
+import org.eclipse.jetty.server.Server;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 @SuppressWarnings("unchecked")
-public class Javalin {
+public class Javalin implements AutoCloseable {
 
     /**
      * Do not use this field unless you know what you're doing.
@@ -192,7 +194,14 @@ public class Javalin {
     /**
      * Synchronously stops the application instance.
      *
+     * Recommended to use {@link Javalin#close} instead with Java's try-with-resources
+     * or Kotlin's {@code use}. This differs from {@link Javalin#close} by
+     * firing lifecycle events even if the server is stopping or already stopped.
+     * This could cause your listeners to observe nonsensical state transitions.
+     * E.g. started -> stopping -> stopped -> stopping -> stopped.
+     *
      * @return stopped application instance.
+     * @see Javalin#close()
      */
     public Javalin stop() {
         JavalinLogger.info("Stopping Javalin ...");
@@ -207,6 +216,20 @@ public class Javalin {
         return this;
     }
 
+    /**
+     * Synchronously stops the application instance.
+     *
+     * Can safely be called multiple times.
+     */
+    @Override
+    public void close() {
+        final Server server = jettyServer.server();
+        if (server.isStopping() || server.isStopped()) {
+            return;
+        }
+        stop();
+    }
+
     public Javalin events(Consumer<EventListener> listener) {
         EventListener eventListener = new EventListener(this.eventManager);
         listener.accept(eventListener);
diff --git a/javalin/src/main/java/io/javalin/http/Context.kt b/javalin/src/main/java/io/javalin/http/Context.kt
index 738869ca..b00c45d6 100644
--- a/javalin/src/main/java/io/javalin/http/Context.kt
+++ b/javalin/src/main/java/io/javalin/http/Context.kt
@@ -439,13 +439,13 @@ open class Context(@JvmField val req: HttpServletRequest, @JvmField val res: Htt
      * Serializes object to a JSON-string using the registered [io.javalin.plugin.json.JsonMapper] and sets it as the context result.
      * Also sets content type to application/json.
      */
-    fun json(obj: Any): Context = result(jsonMapper().toJsonString(obj)).contentType(ContentType.APPLICATION_JSON)
+    fun json(obj: Any): Context = contentType(ContentType.APPLICATION_JSON).result(jsonMapper().toJsonString(obj))
 
     /**
      * Serializes object to a JSON-stream using the registered [io.javalin.plugin.json.JsonMapper] and sets it as the context result.
      * Also sets content type to application/json.
      */
-    fun jsonStream(obj: Any): Context = result(jsonMapper().toJsonStream(obj)).contentType(ContentType.APPLICATION_JSON)
+    fun jsonStream(obj: Any): Context = contentType(ContentType.APPLICATION_JSON).result(jsonMapper().toJsonStream(obj))
 
     /**
      * Renders a file with specified values and sets it as the context result.


Test patch:
diff --git a/javalin/src/test/java/io/javalin/TestClose.kt b/javalin/src/test/java/io/javalin/TestClose.kt
new file mode 100644
index 00000000..216d3319
--- /dev/null
+++ b/javalin/src/test/java/io/javalin/TestClose.kt
@@ -0,0 +1,49 @@
+/*
+ * Javalin - https://javalin.io
+ * Copyright 2017 David Åse
+ * Licensed under Apache 2.0: https://github.com/tipsy/javalin/blob/master/LICENSE
+ *
+ */
+package io.javalin
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+
+class TestClose {
+
+    @Test
+    fun useStopsServer() {
+        val app = Javalin.create()
+
+        app.start(0).use { }
+
+        assertThat(app.jettyServer.server().isStopped).isTrue
+    }
+
+    @Test
+    fun useCallsLifecycleEvents() {
+        var log = ""
+        val app = Javalin.create().events {
+            it.serverStopping { log += "Stopping" }
+            it.serverStopped { log += "Stopped" }
+        }
+
+        app.start(0).use { }
+
+        assertThat(log).isEqualTo("StoppingStopped")
+    }
+
+    @Test
+    fun closingInsideUseIsIdempotent() {
+        var log = ""
+        val app = Javalin.create().events {
+            it.serverStopping { log += "Stopping" }
+            it.serverStopped { log += "Stopped" }
+        }
+
+        app.start(0).use { it.close() }
+
+        assertThat(app.jettyServer.server().isStopped).isTrue
+        assertThat(log).isEqualTo("StoppingStopped")
+    }
+}
diff --git a/javalin/src/test/java/io/javalin/TestClose_Java.java b/javalin/src/test/java/io/javalin/TestClose_Java.java
new file mode 100644
index 00000000..0f994aae
--- /dev/null
+++ b/javalin/src/test/java/io/javalin/TestClose_Java.java
@@ -0,0 +1,64 @@
+/*
+ * Javalin - https://javalin.io
+ * Copyright 2017 David Åse
+ * Licensed under Apache 2.0: https://github.com/tipsy/javalin/blob/master/LICENSE
+ *
+ */
+
+package io.javalin;
+
+import org.eclipse.jetty.server.Server;
+import org.junit.jupiter.api.Test;
+
+import java.util.Objects;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class TestClose_Java {
+
+    @Test
+    void tryWithResourcesStopsServer() {
+        final Javalin app = Javalin.create();
+
+        //noinspection EmptyTryBlock
+        try (final Javalin ignored = app.start(0)) {
+            // do nothing
+        }
+
+        final Server server = Objects.requireNonNull(app.jettyServer()).server();
+        assertThat(server.isStopped()).isTrue();
+    }
+
+    @Test
+    void tryWithResourcesCallsLifecycleEvents() {
+        final StringBuilder log = new StringBuilder();
+        final Javalin app = Javalin.create().events(event -> {
+            event.serverStopping(() -> log.append("Stopping"));
+            event.serverStopped(() -> log.append("Stopped"));
+        });
+
+        //noinspection EmptyTryBlock
+        try (final Javalin ignored = app.start(0)) {
+            // do nothing
+        }
+
+        assertThat(log.toString()).isEqualTo("StoppingStopped");
+    }
+
+    @Test
+    void closingInsideTryWithResourcesIsIdempotent() {
+        final StringBuilder log = new StringBuilder();
+        final Javalin app = Javalin.create().events(event -> {
+            event.serverStopping(() -> log.append("Stopping"));
+            event.serverStopped(() -> log.append("Stopped"));
+        });
+        try (final Javalin startedApp = app.start(0)) {
+            //noinspection RedundantExplicitClose
+            startedApp.close();
+        }
+
+        final Server server = Objects.requireNonNull(app.jettyServer()).server();
+        assertThat(server.isStopped()).isTrue();
+        assertThat(log.toString()).isEqualTo("StoppingStopped");
+    }
+}
diff --git a/javalin/src/test/java/io/javalin/testing/TestUtil.java b/javalin/src/test/java/io/javalin/testing/TestUtil.java
index a3b5d19c..b31b520b 100644
--- a/javalin/src/test/java/io/javalin/testing/TestUtil.java
+++ b/javalin/src/test/java/io/javalin/testing/TestUtil.java
@@ -19,12 +19,12 @@ public class TestUtil {
 
     public static void test(Javalin javalin, ThrowingBiConsumer<Javalin, HttpUtil> test) {
         JavalinLogger.enabled = false;
-        javalin.start(0);
-        HttpUtil http = new HttpUtil(javalin.port());
-        test.accept(javalin, http);
-        javalin.delete("/x-test-cookie-cleaner", ctx -> ctx.cookieMap().keySet().forEach(ctx::removeCookie));
-        http.call(HttpMethod.DELETE, "/x-test-cookie-cleaner");
-        javalin.stop();
+        try (final Javalin closingJavalin = javalin.start(0)) {
+            HttpUtil http = new HttpUtil(closingJavalin.port());
+            test.accept(closingJavalin, http);
+            closingJavalin.delete("/x-test-cookie-cleaner", ctx -> ctx.cookieMap().keySet().forEach(ctx::removeCookie));
+            http.call(HttpMethod.DELETE, "/x-test-cookie-cleaner");
+        }
         JavalinLogger.enabled = true;
     }