diff --git a/android/README.md b/android/README.md index de45edb3..3e60a466 100644 --- a/android/README.md +++ b/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 diff --git a/android/src/androidTest/java/app/opendocument/core/HttpServerTest.kt b/android/src/androidTest/java/app/opendocument/core/HttpServerTest.kt index 81ee66c2..5050fcc9 100644 --- a/android/src/androidTest/java/app/opendocument/core/HttpServerTest.kt +++ b/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()) diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index 99c5d0cb..46890195 100644 --- a/jni/CMakeLists.txt +++ b/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" diff --git a/jni/java/app/opendocument/core/GuardedNativeResource.java b/jni/java/app/opendocument/core/GuardedNativeResource.java new file mode 100644 index 00000000..4ba14711 --- /dev/null +++ b/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. + * + *

{@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. + * + *

{@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(); + } +} diff --git a/jni/java/app/opendocument/core/HttpServer.java b/jni/java/app/opendocument/core/HttpServer.java index cedf342e..5fd15b5e 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/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()}). + * + *

{@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. + * + *

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); diff --git a/jni/java/app/opendocument/core/NativeResource.java b/jni/java/app/opendocument/core/NativeResource.java index 5e08bddf..22c13174 100644 --- a/jni/java/app/opendocument/core/NativeResource.java +++ b/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() { diff --git a/jni/src/jni_http_server.cpp b/jni/src/jni_http_server.cpp index 3868d686..ea4a2913 100644 --- a/jni/src/jni_http_server.cpp +++ b/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(); }); diff --git a/jni/tests/app/opendocument/core/HttpServerTest.java b/jni/tests/app/opendocument/core/HttpServerTest.java index 5ebb1dde..c3f1b3d9 100644 --- a/jni/tests/app/opendocument/core/HttpServerTest.java +++ b/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"); diff --git a/python/src/bind_http_server.cpp b/python/src/bind_http_server.cpp index e55b478c..127155bd 100644 --- a/python/src/bind_http_server.cpp +++ b/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::call_guard(), + "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(), + "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::call_guard(), + "Stop `listen` and release the socket. Blocks until `listen` has " + "returned, so nothing is serving any more once this returns."); } diff --git a/python/tests/test_http_server.py b/python/tests/test_http_server.py index 289bfb8c..0e6a4119 100644 --- a/python/tests/test_http_server.py +++ b/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() diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp index 25e55047..fbdec049 100644 --- a/src/odr/http_server.cpp +++ b/src/odr/http_server.cpp @@ -7,14 +7,38 @@ #include #include +#include +#include +#include +#include +#include #include namespace odr { -class HttpServer::Impl { +class HttpServer::Impl : public std::enable_shared_from_this { public: + /// What a HttpServer holds: a second reference to the impl whose deleter + /// stops the server rather than destroying it. Running out of handles is what + /// has to stop a listen() in flight, and ~Impl cannot be that signal - + /// listen() keeps a reference of its own, so the impl outlives the handles + /// for as long as it serves. Destroying it is left to the reference captured + /// below. + static std::shared_ptr create(const Logger &logger) { + std::shared_ptr owner = std::make_shared(logger); + Impl *const impl = owner.get(); + + // shared_from_this() stays bound to the control block make_shared put + // there: a second one only takes weak_this over when it has expired. So + // listen() holds that reference, not one of these, which is the whole point + return std::shared_ptr{ + impl, [owner = std::move(owner)](Impl * /*owner already has it*/) { + owner->stop(); + }}; + } + explicit Impl(const Logger &logger) - : m_logger{logger}, m_server{std::make_unique()} { + : m_logger{logger}, m_server{std::make_shared()} { // Set up exception handler to catch any internal httplib exceptions. // This prevents crashes when exceptions occur during request processing. m_server->set_exception_handler([this](const httplib::Request & /*req*/, @@ -59,21 +83,12 @@ class HttpServer::Impl { } ~Impl() { - // Ensure the server is properly stopped before destruction. - // This prevents crashes from worker threads accessing freed memory. - // - // IMPORTANT: We must fully stop and destroy the server BEFORE - // m_content is destroyed. httplib's thread pool threads may still - // be running after stop() returns - they only fully stop when the - // Server destructor joins them. By explicitly destroying the server - // here (via unique_ptr::reset), we ensure all threads are joined - // before any other members are destroyed. - m_stopping.store(true, std::memory_order_release); - if (m_server != nullptr) { - m_server->stop(); - m_server.reset(); // Destroy server, join all thread pool threads - } - // Now safe to let other members destruct - no threads are running + // listen() holds a reference to the impl of its own, so this cannot run + // underneath an accept loop. stop() is still what tears the server down: it + // closes the socket, waits for any listen() to return and only then drops + // the httplib server, whose destructor joins the thread pool. m_content is + // destroyed after this body, i.e. after all of that. + stop(); } // Prevent copying - the lambdas capture 'this' so copying would be unsafe @@ -154,12 +169,14 @@ class HttpServer::Impl { std::uint32_t bind(const std::string &host, const std::uint32_t port, const Options &options) { + const std::unique_lock lock{m_mutex}; + if (m_server == nullptr) { throw ServerNotBound(); } // binding twice would overwrite the socket cpp-httplib holds, leaking the // first one and its port for good - if (m_bound.load(std::memory_order_acquire)) { + if (m_bound) { throw ServerAlreadyBound(); } @@ -201,23 +218,52 @@ class HttpServer::Impl { throw ServerBindFailed(host, port); } - m_bound.store(true, std::memory_order_release); + m_bound = true; ODR_VERBOSE(m_logger, "Bound to " << host << ":" << bound); return static_cast(bound); } - void listen() const { - // cpp-httplib's listen_after_bind() reports success for a server that was - // never bound, so the state has to be checked here - if (m_server == nullptr || !m_bound.load(std::memory_order_acquire)) { - throw ServerNotBound(); + void listen() { + // listen() blocks, so it runs on a thread of the caller's. A reference of + // its own keeps the impl - and with it the httplib server, the mutex and + // the condition variable below - alive for as long as the accept loop is on + // it, whatever the thread that owns the HttpServer does meanwhile. + const std::shared_ptr self = shared_from_this(); + + std::shared_ptr server; + + { + std::unique_lock lock{m_mutex}; + + if (m_stopping.load(std::memory_order_acquire)) { + // stop() got here first, so there is nothing left to serve + return; + } + // cpp-httplib's listen_after_bind() reports success for a server that was + // never bound, so the state has to be checked here + if (m_server == nullptr || !m_bound) { + throw ServerNotBound(); + } + + server = m_server; + ++m_listening; } + // right after the count went up, before anything that can throw: stop() + // waits for the count and would never come back otherwise + const ListenGuard guard{*this}; + ODR_VERBOSE(m_logger, "Serving..."); - m_server->listen_after_bind(); + server->listen_after_bind(); + } + + bool is_running() const { + const std::unique_lock lock{m_mutex}; + + return m_listening > 0; } void clear() { @@ -229,58 +275,103 @@ class HttpServer::Impl { } void stop() { - ODR_VERBOSE(m_logger, "Stopping HTTP server..."); - - // Set stopping flag first to reject new requests immediately. - // This prevents new requests from starting while we're shutting down. - m_stopping.store(true, std::memory_order_release); - - if (m_server != nullptr) { - // Stop the server to prevent new connections. - // Note: httplib::Server::stop() signals shutdown but thread pool - // threads may still be running. They only fully stop when the - // Server is destroyed. For explicit stop() calls (not destructor), - // we destroy the server here to ensure threads are joined. - m_server->stop(); - m_server.reset(); // Destroy server, join all thread pool threads - } + // whoever gets here first takes the server away from the impl; a second + // stop() finds nothing to close and only waits below + std::shared_ptr server; - m_bound.store(false, std::memory_order_release); + { + std::unique_lock lock{m_mutex}; + + if (m_stopping.load(std::memory_order_acquire) && m_server == nullptr && + m_listening == 0) { + // stopped already, and nothing left in flight - both the last handle + // and ~Impl get here + return; + } + + ODR_VERBOSE(m_logger, "Stopping HTTP server..."); + + // rejects requests in flight, and any listen() that has not started yet + m_stopping.store(true, std::memory_order_release); + m_bound = false; + server = std::move(m_server); + + if (server != nullptr && m_listening > 0) { + // httplib::Server::stop() closes the listening socket, but only once + // listen_internal() has the accept loop up - and it asserts, then + // closes an already closed descriptor, if it runs a second time after + // that. A listen() that is still on its way there therefore has to be + // waited for, and httplib's own flag is the only thing to wait on. + while (m_listening > 0 && !server->is_running()) { + m_listen_done.wait_for(lock, std::chrono::milliseconds{1}); + } + if (server->is_running()) { + lock.unlock(); + server->stop(); + lock.lock(); + } + } + + // the accept loop stands on the server object and on everything the + // handlers capture, so neither may go before listen() has returned: + // dropping the server underneath it was the use after free, and the two + // then raced over the listening socket as well + m_listen_done.wait(lock, [this] { return m_listening == 0; }); + } - // Clear content after server is fully destroyed to avoid use-after-free. + // nothing is serving any more clear(); + + // ~server here, which joins the thread pool } private: + /// Marks a listen() as done however it leaves, so stop() can wait for it. + struct ListenGuard { + Impl &impl; + + ~ListenGuard() { + const std::unique_lock lock{impl.m_mutex}; + + --impl.m_listening; + // under the lock: the waiter may be tearing the impl down right after + impl.m_listen_done.notify_all(); + } + }; + Logger m_logger; + // guards the lifecycle state below as well as m_content + mutable std::mutex m_mutex; + // signalled when a listen() returns + std::condition_variable m_listen_done; + // listen() calls in flight - 0 or 1 in any sane use + std::size_t m_listening{0}; + // Flag to indicate server is shutting down - checked by handlers - // to reject new requests during shutdown. + // to reject new requests during shutdown. Atomic because they read it + // without the lock. std::atomic m_stopping{false}; // Whether bind() has taken a socket. listen() needs it because cpp-httplib // will happily "serve" a server that never bound one. - std::atomic m_bound{false}; + bool m_bound{false}; struct Content { std::string id; HtmlService service; }; - std::mutex m_mutex; std::unordered_map m_content; - // IMPORTANT: m_server is declared LAST and as unique_ptr so we can - // explicitly destroy it in the destructor BEFORE other members. - // httplib's Server destructor joins thread pool threads, so we must - // ensure threads are fully stopped before m_content is destroyed. - // Using unique_ptr allows us to call reset() to trigger destruction - // at a controlled point in the destructor. - std::unique_ptr m_server; + // Shared rather than unique: listen() takes a reference of its own, so the + // server outlives an overlapping stop() and is destroyed - joining the thread + // pool - only once the accept loop is off it. + std::shared_ptr m_server; }; HttpServer::HttpServer(const Config & /*config*/, const Logger &logger) - : m_impl{std::make_unique(logger)} {} + : m_impl{Impl::create(logger)} {} void HttpServer::connect_service(HtmlService service, const std::string &prefix) const { @@ -309,6 +400,8 @@ std::uint32_t HttpServer::bind(const std::string &host, void HttpServer::listen() const { m_impl->listen(); } +bool HttpServer::is_running() const { return m_impl->is_running(); } + void HttpServer::clear() const { m_impl->clear(); } void HttpServer::stop() const { m_impl->stop(); } diff --git a/src/odr/http_server.hpp b/src/odr/http_server.hpp index 46ebfd70..6a711c1b 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -29,6 +29,12 @@ struct HttpServerOptions { ///< connections between them, hence off }; +/// Serves connected HtmlServices over HTTP. listen() blocks and therefore runs +/// on a thread of the caller's; stop() - and destroying the last handle, which +/// stops the server too - returns only once that thread is out of listen() +/// again, so neither may be called from a request handler. A thread that has +/// not entered listen() yet is invisible to both: as with any call on an object +/// being destroyed, it has to be in before the last handle goes. class HttpServer { public: constexpr static auto prefix_pattern = R"(([a-zA-Z0-9_-]+))"; @@ -50,16 +56,22 @@ class HttpServer { std::uint32_t bind(const std::string &host, std::uint32_t port, const Options &options = {}) const; - /// Serves what bind() opened until stop(). Throws ServerNotBound. + /// Serves what bind() opened until stop(). Returns right away if the server + /// has already been stopped. Throws ServerNotBound. void listen() const; + /// Whether a listen() is in flight. False again once it has returned, which + /// is what stop() waits for. + bool is_running() const; + /// Drops the connected services. Files they were translated into are the /// caller's, and are left alone. void clear() const; - /// Stops listen() and releases the socket. A server that was bound but never - /// listened keeps its port until the process ends - cpp-httplib only closes - /// the socket from its accept loop. + /// Stops listen() and releases the socket, blocking until listen() has + /// returned - nothing is serving any more once this returns. A server that + /// was bound but never listened keeps its port until the process ends - + /// cpp-httplib only closes the socket from its accept loop. void stop() const; private: diff --git a/test/src/http_server_test.cpp b/test/src/http_server_test.cpp index 402b222a..8b3c86a6 100644 --- a/test/src/http_server_test.cpp +++ b/test/src/http_server_test.cpp @@ -3,9 +3,24 @@ #include +#include +#include #include +#include using namespace odr; +using namespace std::chrono_literals; + +namespace { + +/// Spins until listen() is in flight, so a test can stop a server that serves. +void wait_until_running(const HttpServer &server) { + while (!server.is_running()) { + std::this_thread::sleep_for(1ms); + } +} + +} // namespace TEST(HttpServer, bind_reports_the_port_it_got) { const HttpServer server; @@ -44,3 +59,80 @@ TEST(HttpServer, listen_without_bind_is_refused) { // cpp-httplib's listen_after_bind() reports success for this EXPECT_THROW(server.listen(), ServerNotBound); } + +TEST(HttpServer, stop_returns_only_once_listen_has) { + const HttpServer server; + server.bind("127.0.0.1", 0); + + std::atomic returned{false}; + std::thread thread{[&server, &returned] { + server.listen(); + returned.store(true); + }}; + + wait_until_running(server); + server.stop(); + + // the accept loop is gone for good, so tearing the rest down is safe - which + // is what dropping the server underneath it used to get wrong + EXPECT_FALSE(server.is_running()); + + thread.join(); + EXPECT_TRUE(returned.load()); +} + +TEST(HttpServer, stop_before_the_accept_loop_is_up_still_returns) { + // deliberately no wait_until_running: this is the window where cpp-httplib + // has not taken the socket over yet and its own stop() does nothing, so a + // repeat is enough to hang or to close a descriptor twice + for (int i = 0; i < 20; ++i) { + const HttpServer server; + server.bind("127.0.0.1", 0); + + std::thread thread{[&server] { server.listen(); }}; + + server.stop(); + thread.join(); + } +} + +TEST(HttpServer, stop_twice_is_harmless) { + const HttpServer server; + server.bind("127.0.0.1", 0); + + std::thread thread{[&server] { server.listen(); }}; + + wait_until_running(server); + // cpp-httplib's stop() asserts, then closes an already closed descriptor, + // when it runs a second time on a server that is still winding down + server.stop(); + server.stop(); + + thread.join(); +} + +TEST(HttpServer, destroying_the_last_handle_stops_listen) { + std::thread thread; + + { + const HttpServer server; + server.bind("127.0.0.1", 0); + + thread = std::thread{[&server] { server.listen(); }}; + wait_until_running(server); + } + + // the destructor stopped the accept loop and waited for it, so this returns + // rather than blocking forever + thread.join(); +} + +TEST(HttpServer, listen_after_stop_returns) { + const HttpServer server; + 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 + EXPECT_NO_THROW(server.listen()); +}