diff --git a/cli/src/server.cpp b/cli/src/server.cpp index 2f679833..23d39296 100644 --- a/cli/src/server.cpp +++ b/cli/src/server.cpp @@ -5,6 +5,8 @@ #include #include +#include +#include #include #include @@ -40,8 +42,18 @@ int main(const int argc, char **argv) { } } - HttpServer::Config server_config; - HttpServer server(server_config); + const HttpServer server{{}, 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 + 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; @@ -52,12 +64,16 @@ 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, - "http://localhost:8080/file/" << prefix << "/" << view.path()); + ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); } } @@ -68,18 +84,19 @@ 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()) { - 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 fad1ebd4..cedf342e 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/jni/java/app/opendocument/core/HttpServer.java @@ -1,27 +1,36 @@ 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 { + /** 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); + public HttpServer() { + this(new Config()); } - public Config config() { - Config config = new Config(); - config.cachePath = cachePathNative(handle()); - return config; + public HttpServer(Config config) { + super(create(), null, HttpServer::destroy); } /** Hosts the service under {@code //}. */ @@ -29,19 +38,23 @@ 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()} + * 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() { @@ -52,18 +65,16 @@ 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); - private native void listenNative(long handle, String host, int port); + 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 f437ff1d..3868d686 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,13 +27,9 @@ 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) { - return guarded(env, [&] { - odr::HttpServer::Config config; - config.cache_path = to_string(env, cache_path); - return make_handle(odr::HttpServer(config)); - }); +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_HttpServer_create(JNIEnv *env, jclass) { + return guarded(env, [&] { return make_handle(odr::HttpServer()); }); } extern "C" JNIEXPORT void JNICALL @@ -44,13 +37,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,39 +48,25 @@ 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) { +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, [&] { - 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; + 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 @@ -127,19 +99,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, @@ -147,16 +113,16 @@ 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, + 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 559884fa..5ebb1dde 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 @@ -63,24 +59,26 @@ 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(); + // 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 = 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 +101,31 @@ void serveFile() throws Exception { } assertFalse(thread.isAlive()); } + + @Test + void bindReportsWhatItGot() { + assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); + + 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 + 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() { + assumeTrue(Odr.hasHttpServer(), "built without the HTTP server"); + + 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 2eadcd11..e55b478c 100644 --- a/python/src/bind_http_server.cpp +++ b/python/src/bind_http_server.cpp @@ -14,25 +14,40 @@ void odr_python::bind_http_server(py::module_ &m) { py::class_ server(m, "HttpServer", "Serves translated files over HTTP."); - py::class_(server, "Config") + 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`.") .def(py::init<>()) - .def_readwrite("cache_path", &odr::HttpServer::Config::cache_path); + .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); }), - py::arg("config")) - .def("config", &odr::HttpServer::config) + py::arg("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, + 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 0eda97fb..289bfb8c 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: @@ -28,21 +21,22 @@ 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() + # 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 = 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 +46,41 @@ 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(): + server = pyodr.HttpServer() + + 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(): + 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() + # reuse_port defaults off, or the two would share the port + with pytest.raises(RuntimeError): + 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() + + # 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 8ad61f65..70aedc35 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 208afe20..cd0f528b 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 74d1091c..e3d9eb2b 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(); @@ -158,23 +153,80 @@ 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(); + } + +#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 + // 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 + }); +#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() { - 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() { @@ -194,19 +246,23 @@ 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(); } private: - Config m_config; - std::shared_ptr m_logger; // Flag to indicate server is shutting down - checked by handlers // 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; @@ -224,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 { @@ -250,24 +303,13 @@ 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 Options &options) const { + return m_impl->bind(host, port, options); } -void HttpServer::listen(const std::string &host, - const std::uint32_t port) const { - m_impl->listen(host, port); -} +void HttpServer::listen() const { m_impl->listen(); } void HttpServer::clear() const { m_impl->clear(); } diff --git a/src/odr/http_server.hpp b/src/odr/http_server.hpp index 0241457c..32d14675 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -13,31 +13,53 @@ 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_-]+))"; - struct Config { - // TODO remove - std::string cache_path{"/tmp/odr"}; - }; + // 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, + 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. + std::uint32_t bind(const std::string &host, std::uint32_t port, + const Options &options = {}) const; - void listen(const std::string &host, std::uint32_t port) const; + /// 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 + /// 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 0a49a32f..2a392c6e 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 00000000..402b222a --- /dev/null +++ b/test/src/http_server_test.cpp @@ -0,0 +1,46 @@ +#include +#include + +#include + +#include + +using namespace odr; + +TEST(HttpServer, bind_reports_the_port_it_got) { + const HttpServer server; + + 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; + + 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; + const std::uint32_t port = taken.bind("127.0.0.1", 0); + + 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; + + // cpp-httplib's listen_after_bind() reports success for this + EXPECT_THROW(server.listen(), ServerNotBound); +}