fix(http-server): make stop() and destruction wait for listen()

android/README.md

@@ -62,6 +62,12 @@ and permission for plain HTTP on loopback (android blocks cleartext from API 28
 on) — a `networkSecurityConfig` with a `domain-config` for `127.0.0.1` is the
 narrow way to grant it.
 
+`HttpServer.listen()` blocks, so it runs on a thread of the app's. `stop()` and
+`close()` return only once that thread is back out of it, so tearing the server
+down needs no join or timeout of its own — and `close()` on its own is enough,
+it stops the server before freeing it. A server that outlives one document is
+best left listening with `clear()` between workloads: rebinding costs a port.
+
 ## Building
 
 The AAR needs the native libraries first; `build_native.py` cross compiles them

android/src/androidTest/java/app/opendocument/core/HttpServerTest.kt

@@ -95,6 +95,33 @@ class HttpServerTest {
         assertFalse(thread.isAlive)
     }
 
+    /**
+     * Tearing the server down while its accept loop is up. android is where this is least
+     * forgiving: fdsan aborts the process when the listening socket is closed twice, and both
+     * teardown races showed up in an instrumented run (#631).
+     */
+    @Test
+    fun closeStopsListen() {
+        assumeTrue("built without the HTTP server", Odr.hasHttpServer())
+
+        val server = HttpServer()
+        server.bind("127.0.0.1", 0)
+
+        val thread = Thread { server.listen() }
+        thread.isDaemon = true
+        thread.start()
+        while (!server.isRunning) {
+            Thread.sleep(1)
+        }
+
+        // close() stops the server before freeing it, rather than pulling it out from
+        // under the accept loop
+        server.close()
+
+        thread.join(5000)
+        assertFalse(thread.isAlive)
+    }
+
     @Test
     fun bindReportsWhatItGot() {
         assumeTrue("built without the HTTP server", Odr.hasHttpServer())

jni/CMakeLists.txt

@@ -102,6 +102,7 @@ add_jar(odr_java
         "java/app/opendocument/core/Frame.java"
         "java/app/opendocument/core/GlobalParams.java"
         "java/app/opendocument/core/GraphicStyle.java"
+        "java/app/opendocument/core/GuardedNativeResource.java"
         "java/app/opendocument/core/HorizontalAlign.java"
         "java/app/opendocument/core/Html.java"
         "java/app/opendocument/core/HtmlConfig.java"

jni/java/app/opendocument/core/GuardedNativeResource.java

@@ -0,0 +1,96 @@
+package app.opendocument.core;
+
+import java.util.function.LongConsumer;
+
+/**
+ * A {@link NativeResource} whose handle stays valid for the duration of a native
+ * call, even when another thread closes it meanwhile.
+ *
+ * <p>{@link NativeResource#handle()} hands out a raw pointer and {@link #close()}
+ * frees what it points at, so a call that has read the handle but not used it yet
+ * is left holding a dangling one. For nearly every binding that is caller error -
+ * you do not use an object while you close it - and plain {@link NativeResource}
+ * is right, at no cost in fields or locks. It is not caller error where the API
+ * asks for the call to be made on a thread of its own, which is
+ * {@code HttpServer.listen()}: closing the server is exactly how that call is
+ * meant to end.
+ *
+ * <p>{@link #guarded} counts such a call in and out. {@link #close()} stops new
+ * ones from starting, calls {@link #unblock()} to make the ones in flight return,
+ * waits for them, and only then frees. A subclass that leaves {@code unblock()}
+ * alone must guard only calls that return on their own, or closing waits forever.
+ */
+public abstract class GuardedNativeResource extends NativeResource {
+  /** Guards {@link #inFlight} and {@link #closing}. */
+  private final Object lock = new Object();
+
+  /** Guarded calls that have taken the handle and not handed it back yet. */
+  private int inFlight;
+
+  /** Set by {@link #close()} before it frees anything, so no call starts after. */
+  private boolean closing;
+
+  GuardedNativeResource(long handle, Object owner, LongConsumer destroyer) {
+    super(handle, owner, destroyer);
+  }
+
+  /**
+   * Runs {@code call} on the native handle and holds it valid for the duration.
+   * Does nothing if the resource is closing or closed.
+   */
+  protected final void guarded(LongConsumer call) {
+    synchronized (lock) {
+      if (closing) {
+        return;
+      }
+      inFlight++;
+    }
+    try {
+      call.accept(handle());
+    } finally {
+      synchronized (lock) {
+        inFlight--;
+        lock.notifyAll();
+      }
+    }
+  }
+
+  /**
+   * Makes the guarded calls in flight return; called by {@link #close()} before it
+   * waits for them. Does nothing by default, which only suits calls that end on
+   * their own.
+   */
+  protected void unblock() {}
+
+  /** Frees the native object once no guarded call is on it any more. Idempotent. */
+  @Override
+  public synchronized void close() {
+    if (isClosed()) {
+      return;
+    }
+
+    synchronized (lock) {
+      closing = true;
+    }
+
+    unblock();
+
+    boolean interrupted = false;
+    synchronized (lock) {
+      while (inFlight > 0) {
+        try {
+          lock.wait();
+        } catch (InterruptedException e) {
+          // giving up here would leak the native object instead, and the wait is
+          // bounded by unblock() above
+          interrupted = true;
+        }
+      }
+    }
+    if (interrupted) {
+      Thread.currentThread().interrupt();
+    }
+
+    super.close();
+  }
+}

jni/java/app/opendocument/core/HttpServer.java

@@ -4,8 +4,13 @@
  * Serves translated files over HTTP. Mirrors {@code odr::HttpServer}. Only
  * available when the native library was built with the HTTP server
  * ({@link Odr#hasHttpServer()}).
+ *
+ * <p>{@link #listen()} blocks and so runs on a thread of the caller's. {@link #stop()}
+ * and {@link #close()} return only once it is back out of it, so tearing the server
+ * down needs no join or timeout of its own, and {@code try (HttpServer server = new
+ * HttpServer())} is safe around a listen thread.
  */
-public final class HttpServer extends NativeResource {
+public final class HttpServer extends GuardedNativeResource {
   /**
    * Mirrors {@code odr::HttpServer::Config}. Empty since the cache path went with
    * {@code serveFile}: what a service was translated into belongs to whoever
@@ -52,19 +57,48 @@ public int bind(String host, int port, Options options) {
     return bindNative(handle(), host, port, options.reuseAddress, options.reusePort);
   }
 
-  /** Blocks serving requests until {@link #stop()} is called from another thread. */
+  /**
+   * Blocks serving requests until {@link #stop()} is called from another thread.
+   * Returns right away if the server has already been stopped or closed.
+   *
+   * <p>Guarded: this is the one call in the bindings that is meant to be made on a
+   * thread of its own while another closes the object it runs on, so the handle it
+   * takes has to stay valid until it hands it back.
+   */
   public void listen() {
-    listenNative(handle());
+    guarded(this::listenNative);
+  }
+
+  /**
+   * Whether a {@link #listen()} is in flight. False again once it has returned,
+   * which is what {@link #stop()} waits for.
+   */
+  public boolean isRunning() {
+    return isRunningNative(handle());
   }
 
   public void clear() {
     clearNative(handle());
   }
 
+  /**
+   * Stops {@link #listen()} and releases the socket. Blocks until {@code listen}
+   * has returned, so nothing is serving any more once this returns - do not call
+   * it from a request handler.
+   */
   public void stop() {
     stopNative(handle());
   }
 
+  /**
+   * What {@link #close()} calls to bring a {@link #listen()} back: closing the
+   * server is how that call is meant to end, and nothing else would end it.
+   */
+  @Override
+  protected void unblock() {
+    stop();
+  }
+
   private static native long create();
 
   private static native void destroy(long handle);
@@ -76,6 +110,8 @@ private native int bindNative(
 
   private native void listenNative(long handle);
 
+  private native boolean isRunningNative(long handle);
+
   private native void clearNative(long handle);
 
   private native void stopNative(long handle);

jni/java/app/opendocument/core/NativeResource.java

@@ -76,6 +76,11 @@ final Object owner() {
     return owner;
   }
 
+  /** Whether the native object has been freed already. */
+  final boolean isClosed() {
+    return closed;
+  }
+
   /** Frees the native object early. Idempotent. */
   @Override
   public void close() {

jni/src/jni_http_server.cpp

@@ -69,6 +69,14 @@ Java_app_opendocument_core_HttpServer_listenNative(JNIEnv *env, jobject,
   guarded(env, [&] { server(handle).listen(); });
 }
 
+extern "C" JNIEXPORT jboolean JNICALL
+Java_app_opendocument_core_HttpServer_isRunningNative(JNIEnv *env, jobject,
+                                                      jlong handle) {
+  return guarded(env, [&]() -> jboolean {
+    return server(handle).is_running() ? JNI_TRUE : JNI_FALSE;
+  });
+}
+
 extern "C" JNIEXPORT void JNICALL
 Java_app_opendocument_core_HttpServer_clearNative(JNIEnv *env, jobject,
                                                   jlong handle) {
@@ -126,6 +134,12 @@ Java_app_opendocument_core_HttpServer_listenNative(JNIEnv *env, jobject,
   odr_jni::guarded(env, [&] { unsupported(); });
 }
 
+extern "C" JNIEXPORT jboolean JNICALL
+Java_app_opendocument_core_HttpServer_isRunningNative(JNIEnv *env, jobject,
+                                                      jlong) {
+  return odr_jni::guarded(env, [&]() -> jboolean { unsupported(); });
+}
+
 extern "C" JNIEXPORT void JNICALL
 Java_app_opendocument_core_HttpServer_clearNative(JNIEnv *env, jobject, jlong) {
   odr_jni::guarded(env, [&] { unsupported(); });

jni/tests/app/opendocument/core/HttpServerTest.java

@@ -119,6 +119,72 @@ void bindReportsWhatItGot() {
     server.stop();
   }
 
+  /** Starts a listen thread and returns once its accept loop is up. */
+  private static Thread listenInBackground(HttpServer server) throws InterruptedException {
+    Thread thread = new Thread(server::listen);
+    thread.setDaemon(true);
+    thread.start();
+    while (!server.isRunning()) {
+      Thread.sleep(1);
+    }
+    return thread;
+  }
+
+  @Test
+  void stopWaitsForListen() throws Exception {
+    assumeTrue(Odr.hasHttpServer(), "built without the HTTP server");
+
+    HttpServer server = new HttpServer();
+    server.bind("127.0.0.1", 0);
+    Thread thread = listenInBackground(server);
+
+    server.stop();
+    // the accept loop is gone for good by now, which is what makes freeing the
+    // native server right after safe
+    assertFalse(server.isRunning());
+
+    thread.join(Duration.ofSeconds(5).toMillis());
+    assertFalse(thread.isAlive());
+  }
+
+  @Test
+  void closeStopsListen() throws Exception {
+    assumeTrue(Odr.hasHttpServer(), "built without the HTTP server");
+
+    Thread thread;
+    try (HttpServer server = new HttpServer()) {
+      server.bind("127.0.0.1", 0);
+      thread = listenInBackground(server);
+    }
+
+    // close() stopped the server before freeing it, rather than pulling it out
+    // from under the accept loop
+    thread.join(Duration.ofSeconds(5).toMillis());
+    assertFalse(thread.isAlive());
+  }
+
+  @Test
+  void closeRacingTheStartOfListenIsSafe() throws Exception {
+    assumeTrue(Odr.hasHttpServer(), "built without the HTTP server");
+
+    // deliberately no wait for isRunning(): close() may get here while the listen
+    // thread has the handle but has not reached the native call, where stopping
+    // alone would find nothing to wait for and free the server underneath it
+    for (int i = 0; i < 20; i++) {
+      HttpServer server = new HttpServer();
+      server.bind("127.0.0.1", 0);
+
+      Thread thread = new Thread(server::listen);
+      thread.setDaemon(true);
+      thread.start();
+
+      server.close();
+
+      thread.join(Duration.ofSeconds(5).toMillis());
+      assertFalse(thread.isAlive());
+    }
+  }
+
   @Test
   void listenWithoutBindThrows() {
     assumeTrue(Odr.hasHttpServer(), "built without the HTTP server");

python/src/bind_http_server.cpp

@@ -46,10 +46,19 @@ void odr_python::bind_http_server(py::module_ &m) {
           "Bind the socket, `port` 0 for any free one; returns the port it "
           "got. Connections are accepted from here on, before `listen` runs.")
       // Release the GIL: `listen` blocks and `stop` must remain callable from
-      // other Python threads.
+      // other Python threads. `stop` waits for the listen thread, which needs
+      // the GIL back to return out of `listen`, so it has to let go of it too.
       .def("listen", &odr::HttpServer::listen,
-           py::call_guard<py::gil_scoped_release>())
+           py::call_guard<py::gil_scoped_release>(),
+           "Serve what `bind` opened until `stop`. Returns right away if the "
+           "server has already been stopped.")
+      .def("is_running", &odr::HttpServer::is_running,
+           py::call_guard<py::gil_scoped_release>(),
+           "Whether a `listen` is in flight. False again once it has returned, "
+           "which is what `stop` waits for.")
       .def("clear", &odr::HttpServer::clear)
       .def("stop", &odr::HttpServer::stop,
-           py::call_guard<py::gil_scoped_release>());
+           py::call_guard<py::gil_scoped_release>(),
+           "Stop `listen` and release the socket. Blocks until `listen` has "
+           "returned, so nothing is serving any more once this returns.");
 }

python/tests/test_http_server.py

@@ -84,3 +84,33 @@ def test_listen_without_bind_raises():
     # cpp-httplib reports success for this, hence the guard being tested
     with pytest.raises(RuntimeError):
         server.listen()
+
+
+@pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server")
+def test_stop_waits_for_listen():
+    server = pyodr.HttpServer()
+    server.bind("127.0.0.1", 0)
+
+    thread = threading.Thread(target=server.listen, daemon=True)
+    thread.start()
+    while not server.is_running():
+        time.sleep(0.001)
+
+    server.stop()
+    # the accept loop is gone for good by now, which is what makes dropping the
+    # server right after safe
+    assert not server.is_running()
+
+    thread.join(timeout=5.0)
+    assert not thread.is_alive()
+
+
+@pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server")
+def test_listen_after_stop_returns():
+    server = pyodr.HttpServer()
+    server.bind("127.0.0.1", 0)
+    server.stop()
+
+    # a listen thread that starts late finds the server stopped; there is
+    # nothing to serve, but nothing went wrong either
+    server.listen()