From fe3aaad83e96b0ea42e6c75be1cad7bb39f32e75 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 27 Jul 2026 16:54:16 +0200 Subject: [PATCH 1/3] feat(http-server)!: split listen into bind and listen listen(host, port) took the address, blocked, and returned void - discarding the bool cpp-httplib hands it. A failed bind was therefore known inside the library and dropped twice, leaving the caller with a server that was never listening and no way to find out. OpenDocument.droid hit exactly that: both flavors hardcode one port, the second app to start silently had no server, and every document ended up behind chrome's error page. It cost a day to diagnose from the outside. bind() now takes the address, reports failure as ServerBindFailed, and returns the port it actually got - so port 0 becomes usable and callers no longer have to probe for a free port and race with it. listen() serves what bind() opened. Since cpp-httplib calls ::listen() during the bind, connections are accepted into the backlog from the moment bind() returns, which removes the "is it up yet?" window between starting a thread and serving on it. Two states that cpp-httplib handles silently are now errors: - binding twice replaced its socket and leaked the first one, holding that port until the process ended. ServerAlreadyBound. - listen_after_bind() on a server that never bound returns *true* and does nothing at all - the loop condition is simply false. ServerNotBound. Socket options move onto bind() as HttpServer::Options, since they belong to the socket rather than the server. They also correct a default: cpp-httplib sets SO_REUSEPORT where it exists and SO_REUSEADDR only otherwise, which is the wrong way round for a server that gets restarted. Only SO_REUSEADDR lets a port still held by TIME_WAIT sockets be bound again - the failure above - while SO_REUSEPORT hands a second server a share of the connections instead. Now reuse_address defaults on and reuse_port off. Callers updated: the CLI binds before printing its urls and prints the port it got; the jni and python bindings gain bind() and Options; their tests drop the free-port dances they needed to work around the old API. BREAKING CHANGE: HttpServer::listen(host, port) is replaced by bind(host, port) + listen(). Java and Python mirror the split. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017sjMgQ5jJuzvE6ErSNuirb --- cli/src/server.cpp | 14 ++-- .../app/opendocument/core/HttpServer.java | 35 +++++++- jni/src/jni_http_server.cpp | 34 ++++++-- .../app/opendocument/core/HttpServerTest.java | 45 ++++++++--- python/src/bind_http_server.cpp | 19 ++++- python/tests/test_http_server.py | 64 ++++++++++++--- src/odr/exceptions.cpp | 10 +++ src/odr/exceptions.hpp | 16 ++++ src/odr/http_server.cpp | 79 +++++++++++++++++-- src/odr/http_server.hpp | 23 +++++- test/CMakeLists.txt | 6 ++ test/src/http_server_test.cpp | 59 ++++++++++++++ 12 files changed, 360 insertions(+), 44 deletions(-) create mode 100644 test/src/http_server_test.cpp diff --git a/cli/src/server.cpp b/cli/src/server.cpp index 2f6798333..07135fb5d 100644 --- a/cli/src/server.cpp +++ b/cli/src/server.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -43,6 +44,11 @@ int main(const int argc, char **argv) { HttpServer::Config server_config; HttpServer server(server_config); + // bind before anything is printed: the port is only known once the socket + // is, and it is not necessarily the one that was asked for + const std::uint32_t port = server.bind("localhost", 8080); + const std::string base_url = "http://localhost:" + std::to_string(port); + HtmlConfig html_config; html_config.embed_images = false; html_config.embed_shipped_resources = true; @@ -56,8 +62,7 @@ int main(const int argc, char **argv) { server.serve_file(decoded_file, prefix, html_config); ODR_INFO(*logger, "hosted decoded file with id: " << prefix); for (const auto &view : views) { - ODR_INFO(*logger, - "http://localhost:8080/file/" << prefix << "/" << view.path()); + ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); } } @@ -74,12 +79,11 @@ int main(const int argc, char **argv) { server.connect_service(filesystem_service, prefix); ODR_INFO(*logger, "hosted filesystem with id: " << prefix); for (const auto &view : filesystem_service.list_views()) { - ODR_INFO(*logger, - "http://localhost:8080/file/" << prefix << "/" << view.path()); + ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); } } - server.listen("localhost", 8080); + server.listen(); return 0; } catch (const std::exception &e) { diff --git a/jni/java/app/opendocument/core/HttpServer.java b/jni/java/app/opendocument/core/HttpServer.java index fad1ebd48..64cb865bc 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/jni/java/app/opendocument/core/HttpServer.java @@ -14,6 +14,18 @@ public static final class Config { public String cachePath = "/tmp/odr"; } + /** Mirrors {@code odr::HttpServer::Options}: socket options for {@link #bind}. */ + public static final class Options { + /** SO_REUSEADDR, so a port held only by sockets in TIME_WAIT can be bound again. */ + public boolean reuseAddress = true; + + /** + * SO_REUSEPORT where the platform has it. Two live servers on one port share the + * incoming connections between them, hence off. + */ + public boolean reusePort = false; + } + public HttpServer(Config config) { super(create(config.cachePath), null, HttpServer::destroy); } @@ -39,9 +51,23 @@ public List serveFile(DecodedFile file, String prefix, HtmlConfig conf return result; } + /** + * Binds the socket and returns the port it got - pass {@code 0} for any free one. + * Connections are accepted into the backlog from here on, before {@link #listen()} + * runs, so a caller can hand out the port as soon as this returns. + */ + public int bind(String host, int port) { + return bind(host, port, new Options()); + } + + /** Binds with explicit socket options. */ + 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. */ - public void listen(String host, int port) { - listenNative(handle(), host, port); + public void listen() { + listenNative(handle()); } public void clear() { @@ -63,7 +89,10 @@ public void stop() { private native long[] serveFileNative( long handle, long fileHandle, String prefix, HtmlConfig config); - private native void listenNative(long handle, String host, int port); + private native int bindNative( + long handle, String host, int port, boolean reuseAddress, boolean reusePort); + + private native void listenNative(long handle); private native void clearNative(long handle); diff --git a/jni/src/jni_http_server.cpp b/jni/src/jni_http_server.cpp index f437ff1db..ce53fbc93 100644 --- a/jni/src/jni_http_server.cpp +++ b/jni/src/jni_http_server.cpp @@ -87,14 +87,25 @@ Java_app_opendocument_core_HttpServer_serveFileNative(JNIEnv *env, jobject, }); } +extern "C" JNIEXPORT jint JNICALL +Java_app_opendocument_core_HttpServer_bindNative(JNIEnv *env, jobject, + jlong handle, jstring host, + jint port, + jboolean reuse_address, + jboolean reuse_port) { + return guarded(env, [&] { + odr::HttpServer::Options options; + options.reuse_address = reuse_address == JNI_TRUE; + options.reuse_port = reuse_port == JNI_TRUE; + return static_cast(server(handle).bind( + to_string(env, host), static_cast(port), options)); + }); +} + extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_HttpServer_listenNative(JNIEnv *env, jobject, - jlong handle, jstring host, - jint port) { - guarded(env, [&] { - server(handle).listen(to_string(env, host), - static_cast(port)); - }); + jlong handle) { + guarded(env, [&] { server(handle).listen(); }); } extern "C" JNIEXPORT void JNICALL @@ -154,9 +165,16 @@ Java_app_opendocument_core_HttpServer_serveFileNative(JNIEnv *env, jobject, return odr_jni::guarded(env, [&]() -> jlongArray { unsupported(); }); } +extern "C" JNIEXPORT jint JNICALL +Java_app_opendocument_core_HttpServer_bindNative(JNIEnv *env, jobject, jlong, + jstring, jint, jboolean, + jboolean) { + return odr_jni::guarded(env, [&]() -> jint { unsupported(); }); +} + extern "C" JNIEXPORT void JNICALL -Java_app_opendocument_core_HttpServer_listenNative(JNIEnv *env, jobject, jlong, - jstring, jint) { +Java_app_opendocument_core_HttpServer_listenNative(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 559884fa1..3327bc066 100644 --- a/jni/tests/app/opendocument/core/HttpServerTest.java +++ b/jni/tests/app/opendocument/core/HttpServerTest.java @@ -2,15 +2,17 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; -import java.net.ServerSocket; import java.net.URI; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; import java.util.List; @@ -21,12 +23,6 @@ class HttpServerTest { @TempDir Path tempDir; - private static int freePort() throws IOException { - try (ServerSocket socket = new ServerSocket(0)) { - return socket.getLocalPort(); - } - } - private record Response(int status, String body) {} // HttpURLConnection with "Connection: close" so no keep-alive connection @@ -74,13 +70,13 @@ void serveFile() throws Exception { List views = server.serveFile(file, "doc", htmlConfig); assertEquals(1, views.size()); - int port = freePort(); + int port = server.bind("127.0.0.1", 0); AtomicReference listenError = new AtomicReference<>(); Thread thread = new Thread( () -> { try { - server.listen("127.0.0.1", port); + server.listen(); } catch (Throwable t) { listenError.set(t); } @@ -103,4 +99,35 @@ void serveFile() throws Exception { } assertFalse(thread.isAlive()); } + + @Test + void bindReportsWhatItGot() throws IOException { + assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); + + HttpServer.Config config = new HttpServer.Config(); + config.cachePath = Files.createDirectories(tempDir.resolve("server-cache")).toString(); + HttpServer server = new HttpServer(config); + + // a literal address on purpose: "localhost" resolves to both ::1 and 127.0.0.1, + // so a second bind would land on the other one instead of colliding + int port = server.bind("127.0.0.1", 0); + assertNotEquals(0, port); + + // a second bind would leak the first socket, so it is refused + assertThrows(RuntimeException.class, () -> server.bind("127.0.0.1", 0)); + + server.stop(); + } + + @Test + void listenWithoutBindThrows() throws IOException { + assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); + + HttpServer.Config config = new HttpServer.Config(); + config.cachePath = Files.createDirectories(tempDir.resolve("server-cache")).toString(); + HttpServer server = new HttpServer(config); + + // cpp-httplib reports success for this, hence the guard being tested + assertThrows(RuntimeException.class, server::listen); + } } diff --git a/python/src/bind_http_server.cpp b/python/src/bind_http_server.cpp index 2eadcd117..18e91b6c2 100644 --- a/python/src/bind_http_server.cpp +++ b/python/src/bind_http_server.cpp @@ -18,6 +18,12 @@ void odr_python::bind_http_server(py::module_ &m) { .def(py::init<>()) .def_readwrite("cache_path", &odr::HttpServer::Config::cache_path); + py::class_(server, "Options", + "Socket options for `bind`.") + .def(py::init<>()) + .def_readwrite("reuse_address", &odr::HttpServer::Options::reuse_address) + .def_readwrite("reuse_port", &odr::HttpServer::Options::reuse_port); + server .def(py::init([](const odr::HttpServer::Config &config) { return odr::HttpServer(config); @@ -30,9 +36,20 @@ void odr_python::bind_http_server(py::module_ &m) { py::arg("prefix"), py::arg("config"), "Translate a decoded file and host it under " "`/file//`; returns the views.") + .def( + "bind", + [](const odr::HttpServer &self, const std::string &host, + const std::uint32_t port, + const odr::HttpServer::Options &options) { + return self.bind(host, port, options); + }, + py::arg("host"), py::arg("port"), + py::arg("options") = odr::HttpServer::Options{}, + "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. - .def("listen", &odr::HttpServer::listen, py::arg("host"), py::arg("port"), + .def("listen", &odr::HttpServer::listen, py::call_guard()) .def("clear", &odr::HttpServer::clear) .def("stop", &odr::HttpServer::stop, diff --git a/python/tests/test_http_server.py b/python/tests/test_http_server.py index 0eda97fb5..711ff90d3 100644 --- a/python/tests/test_http_server.py +++ b/python/tests/test_http_server.py @@ -1,4 +1,3 @@ -import socket import threading import time import urllib.request @@ -8,12 +7,6 @@ import pyodr -def free_port(): - with socket.socket() as sock: - sock.bind(("localhost", 0)) - return sock.getsockname()[1] - - def fetch(url, timeout=5.0): deadline = time.monotonic() + timeout while True: @@ -39,10 +32,8 @@ def test_serve_file(core_data_path, odt_path, tmp_path): views = server.serve_file(file, "doc", html_config) assert len(views) == 1 - port = free_port() - thread = threading.Thread( - target=server.listen, args=("localhost", port), daemon=True - ) + port = server.bind("localhost", 0) + thread = threading.Thread(target=server.listen, daemon=True) thread.start() try: status, body = fetch(f"http://localhost:{port}/file/doc/{views[0].path()}") @@ -52,3 +43,54 @@ def test_serve_file(core_data_path, odt_path, tmp_path): server.stop() 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_bind_reports_what_it_got(tmp_path): + config = pyodr.HttpServer.Config() + cache_path = tmp_path / "server-cache" + cache_path.mkdir() + config.cache_path = str(cache_path) + server = pyodr.HttpServer(config) + + port = server.bind("127.0.0.1", 0) + assert port != 0 + + # a second bind would leak the first socket, so it is refused + with pytest.raises(RuntimeError): + server.bind("127.0.0.1", 0) + + server.stop() + + +@pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") +def test_bind_reports_a_port_in_use(tmp_path): + config = pyodr.HttpServer.Config() + cache_path = tmp_path / "server-cache" + cache_path.mkdir() + config.cache_path = str(cache_path) + taken = pyodr.HttpServer(config) + # a literal address on purpose: "localhost" resolves to both ::1 and + # 127.0.0.1, so a second bind lands on the other one instead of colliding + port = taken.bind("127.0.0.1", 0) + + other = pyodr.HttpServer(config) + options = pyodr.HttpServer.Options() + options.reuse_port = False # or the two would share the port + with pytest.raises(RuntimeError): + other.bind("127.0.0.1", port, options) + + taken.stop() + + +@pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") +def test_listen_without_bind_raises(tmp_path): + config = pyodr.HttpServer.Config() + cache_path = tmp_path / "server-cache" + cache_path.mkdir() + config.cache_path = str(cache_path) + server = pyodr.HttpServer(config) + + # cpp-httplib reports success for this, hence the guard being tested + with pytest.raises(RuntimeError): + server.listen() diff --git a/src/odr/exceptions.cpp b/src/odr/exceptions.cpp index 8ad61f650..70aedc358 100644 --- a/src/odr/exceptions.cpp +++ b/src/odr/exceptions.cpp @@ -113,6 +113,16 @@ PrefixInUse::PrefixInUse() : std::runtime_error("prefix in use") {} PrefixInUse::PrefixInUse(const std::string &prefix) : std::runtime_error("prefix in use: " + prefix) {} +ServerBindFailed::ServerBindFailed(const std::string &host, + const std::uint32_t port) + : std::runtime_error("server bind failed: " + host + ":" + + std::to_string(port)) {} + +ServerAlreadyBound::ServerAlreadyBound() + : std::runtime_error("server is bound already") {} + +ServerNotBound::ServerNotBound() : std::runtime_error("server is not bound") {} + UnsupportedOption::UnsupportedOption(const std::string &message) : std::runtime_error("unsupported option: " + message) {} diff --git a/src/odr/exceptions.hpp b/src/odr/exceptions.hpp index 208afe208..cd0f528bc 100644 --- a/src/odr/exceptions.hpp +++ b/src/odr/exceptions.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include namespace odr { @@ -195,6 +196,21 @@ struct PrefixInUse final : std::runtime_error { explicit PrefixInUse(const std::string &prefix); }; +/// @brief HTTP server socket could not be bound +struct ServerBindFailed final : std::runtime_error { + ServerBindFailed(const std::string &host, std::uint32_t port); +}; + +/// @brief HTTP server is bound already +struct ServerAlreadyBound final : std::runtime_error { + ServerAlreadyBound(); +}; + +/// @brief HTTP server has not been bound +struct ServerNotBound final : std::runtime_error { + ServerNotBound(); +}; + /// @brief Unsupported option struct UnsupportedOption final : std::runtime_error { explicit UnsupportedOption(const std::string &message); diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp index 74d1091c3..2b53f0818 100644 --- a/src/odr/http_server.cpp +++ b/src/odr/http_server.cpp @@ -158,10 +158,63 @@ class HttpServer::Impl { m_content.emplace(prefix, Content{prefix, std::move(service)}); } - void listen(const std::string &host, const std::uint32_t port) const { - ODR_VERBOSE(*m_logger, "Listening on " << host << ":" << port); + std::uint32_t bind(const std::string &host, const std::uint32_t port, + const Options &options) { + 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)) { + throw ServerAlreadyBound(); + } + + // cpp-httplib's default sets SO_REUSEPORT where it exists and SO_REUSEADDR + // only otherwise, which is the wrong way round for a server that gets + // restarted: only SO_REUSEADDR lets a port held by TIME_WAIT sockets be + // bound again, while SO_REUSEPORT hands a second server a share of the + // connections instead. + // socket_t is not in the httplib namespace in every version, hence auto + m_server->set_socket_options([options](const auto sock) { + constexpr int yes = 1; + const auto *const value = reinterpret_cast(&yes); + + if (options.reuse_address) { + setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, value, sizeof(yes)); + } +#ifdef SO_REUSEPORT + if (options.reuse_port) { + setsockopt(sock, SOL_SOCKET, SO_REUSEPORT, value, sizeof(yes)); + } +#endif + }); + + const int bound = + port == 0 ? m_server->bind_to_any_port(host) + : (m_server->bind_to_port(host, static_cast(port)) + ? static_cast(port) + : -1); + if (bound < 0) { + throw ServerBindFailed(host, port); + } + + m_bound.store(true, std::memory_order_release); + + ODR_VERBOSE(*m_logger, "Bound to " << host << ":" << bound); - m_server->listen(host, static_cast(port)); + 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(); + } + + ODR_VERBOSE(*m_logger, "Serving..."); + + m_server->listen_after_bind(); } void clear() { @@ -194,6 +247,8 @@ class HttpServer::Impl { m_server.reset(); // Destroy server, join all thread pool threads } + m_bound.store(false, std::memory_order_release); + // Clear content after server is fully destroyed to avoid use-after-free. clear(); } @@ -207,6 +262,10 @@ class HttpServer::Impl { // to reject new requests during shutdown. 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}; + struct Content { std::string id; HtmlService service; @@ -264,11 +323,19 @@ HtmlViews HttpServer::serve_file(const DecodedFile &file, return service.list_views(); } -void HttpServer::listen(const std::string &host, - const std::uint32_t port) const { - m_impl->listen(host, port); +std::uint32_t HttpServer::bind(const std::string &host, + const std::uint32_t port) const { + return bind(host, port, Options{}); } +std::uint32_t HttpServer::bind(const std::string &host, + const std::uint32_t port, + const Options &options) const { + return m_impl->bind(host, port, options); +} + +void HttpServer::listen() const { m_impl->listen(); } + 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 0241457c7..3b34cf4b9 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -17,11 +17,21 @@ class HttpServer { public: constexpr static auto prefix_pattern = R"(([a-zA-Z0-9_-]+))"; + // TODO remove together with serve_file, which is all cache_path is for struct Config { // TODO remove std::string cache_path{"/tmp/odr"}; }; + /// Socket options for bind(). + struct Options { + bool reuse_address{true}; ///< SO_REUSEADDR, so a port held only by sockets + ///< in TIME_WAIT can be bound again + bool reuse_port{false}; ///< SO_REUSEPORT where the platform has it. Two + ///< live servers on one port share the incoming + ///< connections between them, hence off + }; + explicit HttpServer(const Config &config, std::shared_ptr logger = Logger::create_null()); @@ -34,10 +44,21 @@ class HttpServer { const std::string &prefix, const HtmlConfig &config) const; - void listen(const std::string &host, std::uint32_t port) const; + /// Binds the socket and returns the port it got - pass port 0 for any free + /// one. Connections are accepted into the backlog from here on, before + /// listen() runs. Throws ServerBindFailed / ServerAlreadyBound. + std::uint32_t bind(const std::string &host, std::uint32_t port) const; + std::uint32_t bind(const std::string &host, std::uint32_t port, + const Options &options) const; + + /// Serves what bind() opened until stop(). Throws ServerNotBound. + void listen() const; 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. void stop() const; private: diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0a49a32fd..2a392c6e6 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -82,6 +82,12 @@ add_executable(odr_test "src/internal/zip/miniz_test.cpp" "src/internal/zip/zip_archive_test.cpp" ) +if (ODR_WITH_HTTP_SERVER) + target_sources(odr_test + PRIVATE + "src/http_server_test.cpp" + ) +endif () target_include_directories(odr_test PRIVATE "src" diff --git a/test/src/http_server_test.cpp b/test/src/http_server_test.cpp new file mode 100644 index 000000000..6d9c54221 --- /dev/null +++ b/test/src/http_server_test.cpp @@ -0,0 +1,59 @@ +#include +#include + +#include + +#include +#include + +using namespace odr; + +namespace { + +HttpServer::Config test_config(const std::string &name) { + HttpServer::Config config; + config.cache_path = (std::filesystem::temp_directory_path() / name).string(); + std::filesystem::create_directories(config.cache_path); + return config; +} + +} // namespace + +TEST(HttpServer, bind_reports_the_port_it_got) { + const HttpServer server(test_config("odr-http-server-test-any-port")); + + const std::uint32_t port = server.bind("127.0.0.1", 0); + EXPECT_NE(port, 0); + + server.stop(); +} + +TEST(HttpServer, bind_twice_is_refused) { + const HttpServer server(test_config("odr-http-server-test-twice")); + + server.bind("127.0.0.1", 0); + // a second bind would replace the socket cpp-httplib holds, leaking the first + // one and its port until the process ends + EXPECT_THROW(server.bind("127.0.0.1", 0), ServerAlreadyBound); + + server.stop(); +} + +TEST(HttpServer, bind_reports_a_port_in_use) { + const HttpServer taken(test_config("odr-http-server-test-taken")); + const std::uint32_t port = taken.bind("127.0.0.1", 0); + + const HttpServer other(test_config("odr-http-server-test-other")); + HttpServer::Options options; + options.reuse_port = false; // or the two would share the port + EXPECT_THROW(other.bind("127.0.0.1", port, options), ServerBindFailed); + + taken.stop(); +} + +TEST(HttpServer, listen_without_bind_is_refused) { + const HttpServer server(test_config("odr-http-server-test-unbound")); + + // cpp-httplib's listen_after_bind() reports success for this + EXPECT_THROW(server.listen(), ServerNotBound); +} From 089d8b8cd14185906a8535c6cde9860571f71efa Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 27 Jul 2026 17:05:42 +0200 Subject: [PATCH 2/3] refactor(http-server)!: empty the config, and keep windows exclusive Config held nothing but cache_path, which existed only for the serve_file marked TODO remove, and for a clear() that deleted the caller's translated files underneath them. Both go: the server hosts what it is given, and what a service was translated into belongs to whoever translated it. Config stays as an empty struct so server-wide settings have somewhere to land. Callers translate for themselves now - html::translate() plus connect_service(), which is what serve_file did anyway - and the CLI picks its own cache directory instead of inheriting one from the server. Also fixes the socket options on windows, from review: the callback replaced cpp-httplib's defaults wholesale, and on windows those include SO_EXCLUSIVEADDRUSE. The flags mean the opposite there - SO_REUSEADDR lets a second live socket take the endpoint over and SO_EXCLUSIVEADDRUSE is what keeps it ours - so setting only SO_REUSEADDR would hand the port away, which is the hazard reuse_port=false exists to avoid. Windows keeps the defaults and Options is documented as posix only. BREAKING CHANGE: HttpServer::Config::cache_path, HttpServer::config() and HttpServer::serve_file() are gone. Translate with html::translate() and host the result with connect_service(). Java and Python mirror this. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017sjMgQ5jJuzvE6ErSNuirb --- cli/src/server.cpp | 25 ++++++-- .../app/opendocument/core/HttpServer.java | 38 +++--------- jni/src/jni_http_server.cpp | 58 ++----------------- .../app/opendocument/core/HttpServerTest.java | 22 ++++--- python/src/bind_http_server.cpp | 14 ++--- python/tests/test_http_server.py | 37 +++++------- src/odr/http_server.cpp | 50 +++++----------- src/odr/http_server.hpp | 20 +++---- test/src/http_server_test.cpp | 22 ++----- 9 files changed, 88 insertions(+), 198 deletions(-) diff --git a/cli/src/server.cpp b/cli/src/server.cpp index 07135fb5d..5c95cbdf3 100644 --- a/cli/src/server.cpp +++ b/cli/src/server.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -41,8 +42,13 @@ int main(const int argc, char **argv) { } } - HttpServer::Config server_config; - HttpServer server(server_config); + const HttpServer server{HttpServer::Config{}, logger}; + + // the server does not own a cache any more, so the translation goes + // somewhere of our choosing + const std::filesystem::path cache_path = + std::filesystem::temp_directory_path() / "odr-server"; + std::filesystem::remove_all(cache_path); // bind before anything is printed: the port is only known once the socket // is, and it is not necessarily the one that was asked for @@ -58,8 +64,13 @@ int main(const int argc, char **argv) { { const std::string prefix = "file"; - const HtmlViews views = - server.serve_file(decoded_file, prefix, html_config); + const std::string prefix_cache_path = (cache_path / prefix).string(); + std::filesystem::create_directories(prefix_cache_path); + + const HtmlService service = + html::translate(decoded_file, prefix_cache_path, html_config, logger); + server.connect_service(service, prefix); + const HtmlViews views = service.list_views(); ODR_INFO(*logger, "hosted decoded file with id: " << prefix); for (const auto &view : views) { ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); @@ -73,9 +84,11 @@ int main(const int argc, char **argv) { : decoded_file.as_archive_file().archive().as_filesystem(); const std::string prefix = "filesystem"; + const std::string prefix_cache_path = (cache_path / prefix).string(); + std::filesystem::create_directories(prefix_cache_path); + const HtmlService filesystem_service = - html::translate(filesystem, server_config.cache_path + "/" + prefix, - html_config, logger); + html::translate(filesystem, prefix_cache_path, html_config, logger); server.connect_service(filesystem_service, prefix); ODR_INFO(*logger, "hosted filesystem with id: " << prefix); for (const auto &view : filesystem_service.list_views()) { diff --git a/jni/java/app/opendocument/core/HttpServer.java b/jni/java/app/opendocument/core/HttpServer.java index 64cb865bc..0e4bff07e 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/jni/java/app/opendocument/core/HttpServer.java @@ -1,18 +1,17 @@ package app.opendocument.core; -import java.util.ArrayList; -import java.util.List; - /** * Serves translated files over HTTP. Mirrors {@code odr::HttpServer}. Only * available when the native library was built with the HTTP server * ({@link Odr#hasHttpServer()}). */ public final class HttpServer extends NativeResource { - /** Mirrors {@code odr::HttpServer::Config}. */ - public static final class Config { - public String cachePath = "/tmp/odr"; - } + /** + * Mirrors {@code odr::HttpServer::Config}. Empty since the cache path went with + * {@code serveFile}: what a service was translated into belongs to whoever + * translated it. + */ + public static final class Config {} /** Mirrors {@code odr::HttpServer::Options}: socket options for {@link #bind}. */ public static final class Options { @@ -27,13 +26,7 @@ public static final class Options { } public HttpServer(Config config) { - super(create(config.cachePath), null, HttpServer::destroy); - } - - public Config config() { - Config config = new Config(); - config.cachePath = cachePathNative(handle()); - return config; + super(create(), null, HttpServer::destroy); } /** Hosts the service under {@code //}. */ @@ -41,16 +34,6 @@ public void connectService(HtmlService service, String prefix) { connectServiceNative(handle(), service.handle(), prefix); } - /** Translates a decoded file and hosts it under {@code /file//}. */ - public List serveFile(DecodedFile file, String prefix, HtmlConfig config) { - long[] handles = serveFileNative(handle(), file.handle(), prefix, config); - List result = new ArrayList<>(handles.length); - for (long h : handles) { - result.add(new HtmlView(h, this)); - } - return result; - } - /** * Binds the socket and returns the port it got - pass {@code 0} for any free one. * Connections are accepted into the backlog from here on, before {@link #listen()} @@ -78,17 +61,12 @@ public void stop() { stopNative(handle()); } - private static native long create(String cachePath); + private static native long create(); private static native void destroy(long handle); - private native String cachePathNative(long handle); - private native void connectServiceNative(long handle, long serviceHandle, String prefix); - private native long[] serveFileNative( - long handle, long fileHandle, String prefix, HtmlConfig config); - private native int bindNative( long handle, String host, int port, boolean reuseAddress, boolean reusePort); diff --git a/jni/src/jni_http_server.cpp b/jni/src/jni_http_server.cpp index ce53fbc93..d213c581e 100644 --- a/jni/src/jni_http_server.cpp +++ b/jni/src/jni_http_server.cpp @@ -8,15 +8,12 @@ #include #include -#include - namespace { using odr_jni::destroy_handle; using odr_jni::from_handle; using odr_jni::guarded; using odr_jni::make_handle; -using odr_jni::to_jstring; using odr_jni::to_string; odr::HttpServer &server(jlong handle) { @@ -30,12 +27,10 @@ Java_app_opendocument_core_Odr_hasHttpServer(JNIEnv *, jclass) { return JNI_TRUE; } -extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_HttpServer_create( - JNIEnv *env, jclass, jstring cache_path) { +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_HttpServer_create(JNIEnv *env, jclass) { return guarded(env, [&] { - odr::HttpServer::Config config; - config.cache_path = to_string(env, cache_path); - return make_handle(odr::HttpServer(config)); + return make_handle(odr::HttpServer(odr::HttpServer::Config{})); }); } @@ -44,13 +39,6 @@ Java_app_opendocument_core_HttpServer_destroy(JNIEnv *, jclass, jlong handle) { destroy_handle(handle); } -extern "C" JNIEXPORT jstring JNICALL -Java_app_opendocument_core_HttpServer_cachePathNative(JNIEnv *env, jobject, - jlong handle) { - return guarded( - env, [&] { return to_jstring(env, server(handle).config().cache_path); }); -} - extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_HttpServer_connectServiceNative(JNIEnv *env, jobject, jlong handle, @@ -62,31 +50,6 @@ Java_app_opendocument_core_HttpServer_connectServiceNative(JNIEnv *env, jobject, }); } -extern "C" JNIEXPORT jlongArray JNICALL -Java_app_opendocument_core_HttpServer_serveFileNative(JNIEnv *env, jobject, - jlong handle, - jlong file_handle, - jstring prefix, - jobject config) { - return guarded(env, [&] { - const odr::HtmlViews views = server(handle).serve_file( - *from_handle(file_handle), to_string(env, prefix), - odr_jni::html_config_from_java(env, config)); - std::vector handles; - handles.reserve(views.size()); - for (const odr::HtmlView &view : views) { - handles.push_back(make_handle(odr::HtmlView(view))); - } - jlongArray result = env->NewLongArray(static_cast(handles.size())); - if (result == nullptr) { - return jlongArray{}; - } - env->SetLongArrayRegion(result, 0, static_cast(handles.size()), - handles.data()); - return result; - }); -} - extern "C" JNIEXPORT jint JNICALL Java_app_opendocument_core_HttpServer_bindNative(JNIEnv *env, jobject, jlong handle, jstring host, @@ -138,19 +101,13 @@ Java_app_opendocument_core_Odr_hasHttpServer(JNIEnv *, jclass) { } extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_HttpServer_create(JNIEnv *env, jclass, jstring) { +Java_app_opendocument_core_HttpServer_create(JNIEnv *env, jclass) { return odr_jni::guarded(env, [&]() -> jlong { unsupported(); }); } extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_HttpServer_destroy(JNIEnv *, jclass, jlong) {} -extern "C" JNIEXPORT jstring JNICALL -Java_app_opendocument_core_HttpServer_cachePathNative(JNIEnv *env, jobject, - jlong) { - return odr_jni::guarded(env, [&]() -> jstring { unsupported(); }); -} - extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_HttpServer_connectServiceNative(JNIEnv *env, jobject, jlong, jlong, @@ -158,13 +115,6 @@ Java_app_opendocument_core_HttpServer_connectServiceNative(JNIEnv *env, jobject, odr_jni::guarded(env, [&] { unsupported(); }); } -extern "C" JNIEXPORT jlongArray JNICALL -Java_app_opendocument_core_HttpServer_serveFileNative(JNIEnv *env, jobject, - jlong, jlong, jstring, - jobject) { - return odr_jni::guarded(env, [&]() -> jlongArray { unsupported(); }); -} - extern "C" JNIEXPORT jint JNICALL Java_app_opendocument_core_HttpServer_bindNative(JNIEnv *env, jobject, jlong, jstring, jint, jboolean, diff --git a/jni/tests/app/opendocument/core/HttpServerTest.java b/jni/tests/app/opendocument/core/HttpServerTest.java index 3327bc066..1ffd344ba 100644 --- a/jni/tests/app/opendocument/core/HttpServerTest.java +++ b/jni/tests/app/opendocument/core/HttpServerTest.java @@ -59,15 +59,17 @@ void serveFile() throws Exception { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); assumeTrue(TestFiles.hasCoreData(), "odr core data path not available"); - HttpServer.Config config = new HttpServer.Config(); - config.cachePath = tempDir.resolve("server-cache").toString(); - HttpServer server = new HttpServer(config); + HttpServer server = new HttpServer(new HttpServer.Config()); + // the server hosts what it is given; translating is the caller's business + String cachePath = Files.createDirectories(tempDir.resolve("doc-cache")).toString(); DecodedFile file = Odr.open(TestFiles.odtFile(tempDir).toString()); HtmlConfig htmlConfig = new HtmlConfig(); htmlConfig.embedImages = false; htmlConfig.relativeResourcePaths = false; - List views = server.serveFile(file, "doc", htmlConfig); + HtmlService service = Html.translate(file, cachePath, htmlConfig); + server.connectService(service, "doc"); + List views = service.listViews(); assertEquals(1, views.size()); int port = server.bind("127.0.0.1", 0); @@ -101,12 +103,10 @@ void serveFile() throws Exception { } @Test - void bindReportsWhatItGot() throws IOException { + void bindReportsWhatItGot() { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); - HttpServer.Config config = new HttpServer.Config(); - config.cachePath = Files.createDirectories(tempDir.resolve("server-cache")).toString(); - HttpServer server = new HttpServer(config); + HttpServer server = new HttpServer(new HttpServer.Config()); // a literal address on purpose: "localhost" resolves to both ::1 and 127.0.0.1, // so a second bind would land on the other one instead of colliding @@ -120,12 +120,10 @@ void bindReportsWhatItGot() throws IOException { } @Test - void listenWithoutBindThrows() throws IOException { + void listenWithoutBindThrows() { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); - HttpServer.Config config = new HttpServer.Config(); - config.cachePath = Files.createDirectories(tempDir.resolve("server-cache")).toString(); - HttpServer server = new HttpServer(config); + HttpServer server = new HttpServer(new HttpServer.Config()); // cpp-httplib reports success for this, hence the guard being tested assertThrows(RuntimeException.class, server::listen); diff --git a/python/src/bind_http_server.cpp b/python/src/bind_http_server.cpp index 18e91b6c2..2498c4d48 100644 --- a/python/src/bind_http_server.cpp +++ b/python/src/bind_http_server.cpp @@ -14,9 +14,12 @@ void odr_python::bind_http_server(py::module_ &m) { py::class_ server(m, "HttpServer", "Serves translated files over HTTP."); - py::class_(server, "Config") - .def(py::init<>()) - .def_readwrite("cache_path", &odr::HttpServer::Config::cache_path); + py::class_( + server, "Config", + "Server-wide settings. Empty since the cache path went with " + "`serve_file`: " + "what a service was translated into belongs to whoever translated it.") + .def(py::init<>()); py::class_(server, "Options", "Socket options for `bind`.") @@ -29,13 +32,8 @@ void odr_python::bind_http_server(py::module_ &m) { return odr::HttpServer(config); }), py::arg("config")) - .def("config", &odr::HttpServer::config) .def("connect_service", &odr::HttpServer::connect_service, py::arg("service"), py::arg("prefix")) - .def("serve_file", &odr::HttpServer::serve_file, py::arg("file"), - py::arg("prefix"), py::arg("config"), - "Translate a decoded file and host it under " - "`/file//`; returns the views.") .def( "bind", [](const odr::HttpServer &self, const std::string &host, diff --git a/python/tests/test_http_server.py b/python/tests/test_http_server.py index 711ff90d3..b4f1ebcfb 100644 --- a/python/tests/test_http_server.py +++ b/python/tests/test_http_server.py @@ -21,15 +21,18 @@ def fetch(url, timeout=5.0): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") def test_serve_file(core_data_path, odt_path, tmp_path): - config = pyodr.HttpServer.Config() - config.cache_path = str(tmp_path / "server-cache") - server = pyodr.HttpServer(config) + server = pyodr.HttpServer(pyodr.HttpServer.Config()) + # the server hosts what it is given; translating is the caller's business + cache_path = tmp_path / "doc-cache" + cache_path.mkdir() file = pyodr.open(str(odt_path)) html_config = pyodr.HtmlConfig() html_config.embed_images = False html_config.relative_resource_paths = False - views = server.serve_file(file, "doc", html_config) + service = pyodr.html.translate(file, str(cache_path), html_config) + server.connect_service(service, "doc") + views = service.list_views() assert len(views) == 1 port = server.bind("localhost", 0) @@ -46,12 +49,8 @@ def test_serve_file(core_data_path, odt_path, tmp_path): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") -def test_bind_reports_what_it_got(tmp_path): - config = pyodr.HttpServer.Config() - cache_path = tmp_path / "server-cache" - cache_path.mkdir() - config.cache_path = str(cache_path) - server = pyodr.HttpServer(config) +def test_bind_reports_what_it_got(): + server = pyodr.HttpServer(pyodr.HttpServer.Config()) port = server.bind("127.0.0.1", 0) assert port != 0 @@ -64,17 +63,13 @@ def test_bind_reports_what_it_got(tmp_path): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") -def test_bind_reports_a_port_in_use(tmp_path): - config = pyodr.HttpServer.Config() - cache_path = tmp_path / "server-cache" - cache_path.mkdir() - config.cache_path = str(cache_path) - taken = pyodr.HttpServer(config) +def test_bind_reports_a_port_in_use(): + taken = pyodr.HttpServer(pyodr.HttpServer.Config()) # a literal address on purpose: "localhost" resolves to both ::1 and # 127.0.0.1, so a second bind lands on the other one instead of colliding port = taken.bind("127.0.0.1", 0) - other = pyodr.HttpServer(config) + other = pyodr.HttpServer(pyodr.HttpServer.Config()) options = pyodr.HttpServer.Options() options.reuse_port = False # or the two would share the port with pytest.raises(RuntimeError): @@ -84,12 +79,8 @@ def test_bind_reports_a_port_in_use(tmp_path): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") -def test_listen_without_bind_raises(tmp_path): - config = pyodr.HttpServer.Config() - cache_path = tmp_path / "server-cache" - cache_path.mkdir() - config.cache_path = str(cache_path) - server = pyodr.HttpServer(config) +def test_listen_without_bind_raises(): + server = pyodr.HttpServer(pyodr.HttpServer.Config()) # cpp-httplib reports success for this, hence the guard being tested with pytest.raises(RuntimeError): diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp index 2b53f0818..131eae377 100644 --- a/src/odr/http_server.cpp +++ b/src/odr/http_server.cpp @@ -7,15 +7,14 @@ #include #include -#include #include namespace odr { class HttpServer::Impl { public: - Impl(Config config, std::shared_ptr logger) - : m_config{std::move(config)}, m_logger{std::move(logger)}, + explicit Impl(std::shared_ptr logger) + : m_logger{std::move(logger)}, m_server{std::make_unique()} { // Set up exception handler to catch any internal httplib exceptions. // This prevents crashes when exceptions occur during request processing. @@ -82,10 +81,6 @@ class HttpServer::Impl { Impl(const Impl &) = delete; Impl &operator=(const Impl &) = delete; - const Config &config() const { return m_config; } - - const std::shared_ptr &logger() const { return m_logger; } - void serve_file(const httplib::Request &req, httplib::Response &res) { try { std::string id = req.matches[1].str(); @@ -169,6 +164,14 @@ class HttpServer::Impl { throw ServerAlreadyBound(); } +#ifdef _WIN32 + // Windows keeps cpp-httplib's defaults, which set SO_EXCLUSIVEADDRUSE + // alongside SO_REUSEADDR. The two flags mean the opposite of what they do + // below: there SO_REUSEADDR lets a second live socket take the endpoint + // over, and SO_EXCLUSIVEADDRUSE is what keeps it ours. Replacing that with + // the posix mapping would hand the port away, so Options does not apply. + static_cast(options); +#else // cpp-httplib's default sets SO_REUSEPORT where it exists and SO_REUSEADDR // only otherwise, which is the wrong way round for a server that gets // restarted: only SO_REUSEADDR lets a port held by TIME_WAIT sockets be @@ -188,6 +191,7 @@ class HttpServer::Impl { } #endif }); +#endif const int bound = port == 0 ? m_server->bind_to_any_port(host) @@ -218,16 +222,11 @@ class HttpServer::Impl { } void clear() { - ODR_VERBOSE(*m_logger, "Clearing HTTP server cache..."); + ODR_VERBOSE(*m_logger, "Dropping connected services..."); std::unique_lock lock{m_mutex}; m_content.clear(); - - for (const auto &entry : - std::filesystem::directory_iterator(m_config.cache_path)) { - std::filesystem::remove_all(entry.path()); - } } void stop() { @@ -254,8 +253,6 @@ class HttpServer::Impl { } private: - Config m_config; - std::shared_ptr m_logger; // Flag to indicate server is shutting down - checked by handlers @@ -283,12 +280,9 @@ class HttpServer::Impl { std::unique_ptr m_server; }; -HttpServer::HttpServer(const Config &config, std::shared_ptr logger) - : m_impl{std::make_unique(config, std::move(logger))} {} - -const HttpServer::Config &HttpServer::config() const { - return m_impl->config(); -} +HttpServer::HttpServer(const Config & /*config*/, + std::shared_ptr logger) + : m_impl{std::make_unique(std::move(logger))} {} void HttpServer::connect_service(HtmlService service, const std::string &prefix) const { @@ -309,20 +303,6 @@ void HttpServer::connect_service(HtmlService service, m_impl->connect_service(std::move(service), prefix); } -HtmlViews HttpServer::serve_file(const DecodedFile &file, - const std::string &prefix, - const HtmlConfig &config) const { - const std::string cache_path = m_impl->config().cache_path + "/" + prefix; - std::filesystem::create_directories(cache_path); - - const HtmlService service = - html::translate(file, cache_path, config, m_impl->logger()); - - connect_service(service, prefix); - - return service.list_views(); -} - std::uint32_t HttpServer::bind(const std::string &host, const std::uint32_t port) const { return bind(host, port, Options{}); diff --git a/src/odr/http_server.hpp b/src/odr/http_server.hpp index 3b34cf4b9..084779fa7 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -17,13 +17,12 @@ class HttpServer { public: constexpr static auto prefix_pattern = R"(([a-zA-Z0-9_-]+))"; - // TODO remove together with serve_file, which is all cache_path is for - struct Config { - // TODO remove - std::string cache_path{"/tmp/odr"}; - }; + /// Server-wide settings. Empty since cache_path went with serve_file: what a + /// service was translated into belongs to whoever translated it. + struct Config {}; - /// Socket options for bind(). + /// Socket options for bind(). POSIX only: Windows keeps cpp-httplib's + /// exclusive-address defaults, where these flags mean the opposite. struct Options { bool reuse_address{true}; ///< SO_REUSEADDR, so a port held only by sockets ///< in TIME_WAIT can be bound again @@ -35,15 +34,8 @@ class HttpServer { explicit HttpServer(const Config &config, std::shared_ptr logger = Logger::create_null()); - [[nodiscard]] const Config &config() const; - void connect_service(HtmlService service, const std::string &prefix) const; - // TODO remove - [[nodiscard]] HtmlViews serve_file(const DecodedFile &file, - const std::string &prefix, - const HtmlConfig &config) const; - /// Binds the socket and returns the port it got - pass port 0 for any free /// one. Connections are accepted into the backlog from here on, before /// listen() runs. Throws ServerBindFailed / ServerAlreadyBound. @@ -54,6 +46,8 @@ class HttpServer { /// Serves what bind() opened until stop(). Throws ServerNotBound. void listen() 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 diff --git a/test/src/http_server_test.cpp b/test/src/http_server_test.cpp index 6d9c54221..343164adf 100644 --- a/test/src/http_server_test.cpp +++ b/test/src/http_server_test.cpp @@ -4,23 +4,11 @@ #include #include -#include using namespace odr; -namespace { - -HttpServer::Config test_config(const std::string &name) { - HttpServer::Config config; - config.cache_path = (std::filesystem::temp_directory_path() / name).string(); - std::filesystem::create_directories(config.cache_path); - return config; -} - -} // namespace - TEST(HttpServer, bind_reports_the_port_it_got) { - const HttpServer server(test_config("odr-http-server-test-any-port")); + const HttpServer server(HttpServer::Config{}); const std::uint32_t port = server.bind("127.0.0.1", 0); EXPECT_NE(port, 0); @@ -29,7 +17,7 @@ TEST(HttpServer, bind_reports_the_port_it_got) { } TEST(HttpServer, bind_twice_is_refused) { - const HttpServer server(test_config("odr-http-server-test-twice")); + const HttpServer server(HttpServer::Config{}); server.bind("127.0.0.1", 0); // a second bind would replace the socket cpp-httplib holds, leaking the first @@ -40,10 +28,10 @@ TEST(HttpServer, bind_twice_is_refused) { } TEST(HttpServer, bind_reports_a_port_in_use) { - const HttpServer taken(test_config("odr-http-server-test-taken")); + const HttpServer taken(HttpServer::Config{}); const std::uint32_t port = taken.bind("127.0.0.1", 0); - const HttpServer other(test_config("odr-http-server-test-other")); + const HttpServer other(HttpServer::Config{}); HttpServer::Options options; options.reuse_port = false; // or the two would share the port EXPECT_THROW(other.bind("127.0.0.1", port, options), ServerBindFailed); @@ -52,7 +40,7 @@ TEST(HttpServer, bind_reports_a_port_in_use) { } TEST(HttpServer, listen_without_bind_is_refused) { - const HttpServer server(test_config("odr-http-server-test-unbound")); + const HttpServer server(HttpServer::Config{}); // cpp-httplib's listen_after_bind() reports success for this EXPECT_THROW(server.listen(), ServerNotBound); From 2a4b4cfe647bc458a5b13bab3cab2217abc6497a Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 27 Jul 2026 17:21:49 +0200 Subject: [PATCH 3/3] refactor(http-server): default config and options, one bind Both structs move to namespace scope, next to HtmlConfig which is spelled that way already, and the class keeps them as aliases. That is what lets them be defaulted with `= {}` in the header: a nested class's member initializers are not parsed until the enclosing class is complete, so a default argument inside the class cannot reach them - which is why bind() needed two overloads before. One bind() now, with `options = {}`, and a server that can be constructed without arguments at all. Callers, tests and the CLI take the defaults instead of spelling out empty structs, and the java binding gains the matching no-arg constructor while python defaults its config argument. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017sjMgQ5jJuzvE6ErSNuirb --- cli/src/server.cpp | 2 +- .../app/opendocument/core/HttpServer.java | 4 ++ jni/src/jni_http_server.cpp | 4 +- .../app/opendocument/core/HttpServerTest.java | 6 +-- python/src/bind_http_server.cpp | 2 +- python/tests/test_http_server.py | 15 ++++--- src/odr/http_server.cpp | 5 --- src/odr/http_server.hpp | 41 +++++++++++-------- test/src/http_server_test.cpp | 15 ++++--- 9 files changed, 48 insertions(+), 46 deletions(-) diff --git a/cli/src/server.cpp b/cli/src/server.cpp index 5c95cbdf3..23d392962 100644 --- a/cli/src/server.cpp +++ b/cli/src/server.cpp @@ -42,7 +42,7 @@ int main(const int argc, char **argv) { } } - const HttpServer server{HttpServer::Config{}, logger}; + const HttpServer server{{}, logger}; // the server does not own a cache any more, so the translation goes // somewhere of our choosing diff --git a/jni/java/app/opendocument/core/HttpServer.java b/jni/java/app/opendocument/core/HttpServer.java index 0e4bff07e..cedf342e8 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/jni/java/app/opendocument/core/HttpServer.java @@ -25,6 +25,10 @@ public static final class Options { public boolean reusePort = false; } + public HttpServer() { + this(new Config()); + } + public HttpServer(Config config) { super(create(), null, HttpServer::destroy); } diff --git a/jni/src/jni_http_server.cpp b/jni/src/jni_http_server.cpp index d213c581e..3868d6867 100644 --- a/jni/src/jni_http_server.cpp +++ b/jni/src/jni_http_server.cpp @@ -29,9 +29,7 @@ Java_app_opendocument_core_Odr_hasHttpServer(JNIEnv *, jclass) { extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_HttpServer_create(JNIEnv *env, jclass) { - return guarded(env, [&] { - return make_handle(odr::HttpServer(odr::HttpServer::Config{})); - }); + return guarded(env, [&] { return make_handle(odr::HttpServer()); }); } extern "C" JNIEXPORT void JNICALL diff --git a/jni/tests/app/opendocument/core/HttpServerTest.java b/jni/tests/app/opendocument/core/HttpServerTest.java index 1ffd344ba..5ebb1dde3 100644 --- a/jni/tests/app/opendocument/core/HttpServerTest.java +++ b/jni/tests/app/opendocument/core/HttpServerTest.java @@ -59,7 +59,7 @@ void serveFile() throws Exception { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); assumeTrue(TestFiles.hasCoreData(), "odr core data path not available"); - HttpServer server = new HttpServer(new HttpServer.Config()); + HttpServer server = new HttpServer(); // the server hosts what it is given; translating is the caller's business String cachePath = Files.createDirectories(tempDir.resolve("doc-cache")).toString(); @@ -106,7 +106,7 @@ void serveFile() throws Exception { void bindReportsWhatItGot() { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); - HttpServer server = new HttpServer(new HttpServer.Config()); + HttpServer server = new HttpServer(); // a literal address on purpose: "localhost" resolves to both ::1 and 127.0.0.1, // so a second bind would land on the other one instead of colliding @@ -123,7 +123,7 @@ void bindReportsWhatItGot() { void listenWithoutBindThrows() { assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); - HttpServer server = new HttpServer(new HttpServer.Config()); + HttpServer server = new HttpServer(); // cpp-httplib reports success for this, hence the guard being tested assertThrows(RuntimeException.class, server::listen); diff --git a/python/src/bind_http_server.cpp b/python/src/bind_http_server.cpp index 2498c4d48..e55b478c3 100644 --- a/python/src/bind_http_server.cpp +++ b/python/src/bind_http_server.cpp @@ -31,7 +31,7 @@ void odr_python::bind_http_server(py::module_ &m) { .def(py::init([](const odr::HttpServer::Config &config) { return odr::HttpServer(config); }), - py::arg("config")) + py::arg("config") = odr::HttpServer::Config{}) .def("connect_service", &odr::HttpServer::connect_service, py::arg("service"), py::arg("prefix")) .def( diff --git a/python/tests/test_http_server.py b/python/tests/test_http_server.py index b4f1ebcfb..289bfb8cc 100644 --- a/python/tests/test_http_server.py +++ b/python/tests/test_http_server.py @@ -21,7 +21,7 @@ def fetch(url, timeout=5.0): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") def test_serve_file(core_data_path, odt_path, tmp_path): - server = pyodr.HttpServer(pyodr.HttpServer.Config()) + server = pyodr.HttpServer() # the server hosts what it is given; translating is the caller's business cache_path = tmp_path / "doc-cache" @@ -50,7 +50,7 @@ def test_serve_file(core_data_path, odt_path, tmp_path): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") def test_bind_reports_what_it_got(): - server = pyodr.HttpServer(pyodr.HttpServer.Config()) + server = pyodr.HttpServer() port = server.bind("127.0.0.1", 0) assert port != 0 @@ -64,23 +64,22 @@ def test_bind_reports_what_it_got(): @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") def test_bind_reports_a_port_in_use(): - taken = pyodr.HttpServer(pyodr.HttpServer.Config()) + taken = pyodr.HttpServer() # a literal address on purpose: "localhost" resolves to both ::1 and # 127.0.0.1, so a second bind lands on the other one instead of colliding port = taken.bind("127.0.0.1", 0) - other = pyodr.HttpServer(pyodr.HttpServer.Config()) - options = pyodr.HttpServer.Options() - options.reuse_port = False # or the two would share the port + other = pyodr.HttpServer() + # reuse_port defaults off, or the two would share the port with pytest.raises(RuntimeError): - other.bind("127.0.0.1", port, options) + other.bind("127.0.0.1", port) taken.stop() @pytest.mark.skipif(not pyodr.has_http_server, reason="built without the HTTP server") def test_listen_without_bind_raises(): - server = pyodr.HttpServer(pyodr.HttpServer.Config()) + server = pyodr.HttpServer() # cpp-httplib reports success for this, hence the guard being tested with pytest.raises(RuntimeError): diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp index 131eae377..e3d9eb2b9 100644 --- a/src/odr/http_server.cpp +++ b/src/odr/http_server.cpp @@ -303,11 +303,6 @@ void HttpServer::connect_service(HtmlService service, m_impl->connect_service(std::move(service), prefix); } -std::uint32_t HttpServer::bind(const std::string &host, - const std::uint32_t port) const { - return bind(host, port, Options{}); -} - std::uint32_t HttpServer::bind(const std::string &host, const std::uint32_t port, const Options &options) const { diff --git a/src/odr/http_server.hpp b/src/odr/http_server.hpp index 084779fa7..32d146757 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -13,25 +13,33 @@ class Filesystem; struct HtmlConfig; class HtmlService; +/// Server-wide settings for HttpServer. Empty since cache_path went with +/// serve_file: what a service was translated into belongs to whoever +/// translated it. +struct HttpServerConfig {}; + +/// Socket options for HttpServer::bind(). POSIX only: Windows keeps +/// cpp-httplib's exclusive-address defaults, where these flags mean the +/// opposite. +struct HttpServerOptions { + bool reuse_address{true}; ///< SO_REUSEADDR, so a port held only by sockets in + ///< TIME_WAIT can be bound again + bool reuse_port{false}; ///< SO_REUSEPORT where the platform has it. Two + ///< live servers on one port share the incoming + ///< connections between them, hence off +}; + class HttpServer { public: constexpr static auto prefix_pattern = R"(([a-zA-Z0-9_-]+))"; - /// Server-wide settings. Empty since cache_path went with serve_file: what a - /// service was translated into belongs to whoever translated it. - struct Config {}; - - /// Socket options for bind(). POSIX only: Windows keeps cpp-httplib's - /// exclusive-address defaults, where these flags mean the opposite. - struct Options { - bool reuse_address{true}; ///< SO_REUSEADDR, so a port held only by sockets - ///< in TIME_WAIT can be bound again - bool reuse_port{false}; ///< SO_REUSEPORT where the platform has it. Two - ///< live servers on one port share the incoming - ///< connections between them, hence off - }; - - explicit HttpServer(const Config &config, + // at namespace scope, like HtmlConfig, so both can be defaulted with `= {}` + // below: a nested class's member initializers are not parsed until the + // enclosing class is complete, which a default argument here cannot wait for + using Config = HttpServerConfig; + using Options = HttpServerOptions; + + explicit HttpServer(const Config &config = {}, std::shared_ptr logger = Logger::create_null()); void connect_service(HtmlService service, const std::string &prefix) const; @@ -39,9 +47,8 @@ class HttpServer { /// Binds the socket and returns the port it got - pass port 0 for any free /// one. Connections are accepted into the backlog from here on, before /// listen() runs. Throws ServerBindFailed / ServerAlreadyBound. - std::uint32_t bind(const std::string &host, std::uint32_t port) const; std::uint32_t bind(const std::string &host, std::uint32_t port, - const Options &options) const; + const Options &options = {}) const; /// Serves what bind() opened until stop(). Throws ServerNotBound. void listen() const; diff --git a/test/src/http_server_test.cpp b/test/src/http_server_test.cpp index 343164adf..402b222a5 100644 --- a/test/src/http_server_test.cpp +++ b/test/src/http_server_test.cpp @@ -8,7 +8,7 @@ using namespace odr; TEST(HttpServer, bind_reports_the_port_it_got) { - const HttpServer server(HttpServer::Config{}); + const HttpServer server; const std::uint32_t port = server.bind("127.0.0.1", 0); EXPECT_NE(port, 0); @@ -17,7 +17,7 @@ TEST(HttpServer, bind_reports_the_port_it_got) { } TEST(HttpServer, bind_twice_is_refused) { - const HttpServer server(HttpServer::Config{}); + const HttpServer server; server.bind("127.0.0.1", 0); // a second bind would replace the socket cpp-httplib holds, leaking the first @@ -28,19 +28,18 @@ TEST(HttpServer, bind_twice_is_refused) { } TEST(HttpServer, bind_reports_a_port_in_use) { - const HttpServer taken(HttpServer::Config{}); + const HttpServer taken; const std::uint32_t port = taken.bind("127.0.0.1", 0); - const HttpServer other(HttpServer::Config{}); - HttpServer::Options options; - options.reuse_port = false; // or the two would share the port - EXPECT_THROW(other.bind("127.0.0.1", port, options), ServerBindFailed); + const HttpServer other; + // reuse_port defaults off, or the two would share the port + EXPECT_THROW(other.bind("127.0.0.1", port), ServerBindFailed); taken.stop(); } TEST(HttpServer, listen_without_bind_is_refused) { - const HttpServer server(HttpServer::Config{}); + const HttpServer server; // cpp-httplib's listen_after_bind() reports success for this EXPECT_THROW(server.listen(), ServerNotBound);