Merge branch 'main' into rework-document-tree

Author: andiwand

src/odr/http_server.cpp

@@ -8,46 +8,111 @@
 
 #include <httplib/httplib.h>
 
+#include <atomic>
+#include <memory>
+#include <sstream>
+
 namespace odr {
 
 class HttpServer::Impl {
 public:
   Impl(Config config, std::shared_ptr<Logger> logger)
-      : m_config{std::move(config)}, m_logger{std::move(logger)} {
-    m_server.Get("/",
-                 [](const httplib::Request & /*req*/, httplib::Response &res) {
-                   res.set_content("Hello World!", "text/plain");
-                 });
-
-    m_server.Get("/file/" + std::string(prefix_pattern),
-                 [&](const httplib::Request &req, httplib::Response &res) {
-                   serve_file(req, res);
-                 });
-    m_server.Get("/file/" + std::string(prefix_pattern) + "/(.*)",
-                 [&](const httplib::Request &req, httplib::Response &res) {
-                   serve_file(req, res);
-                 });
+      : m_config{std::move(config)}, m_logger{std::move(logger)},
+        m_server{std::make_unique<httplib::Server>()} {
+    // Set up exception handler to catch any internal httplib exceptions.
+    // This prevents crashes when exceptions occur during request processing.
+    m_server->set_exception_handler([this](const httplib::Request & /*req*/,
+                                           httplib::Response &res,
+                                           std::exception_ptr ep) {
+      try {
+        if (ep) {
+          std::rethrow_exception(ep);
+        }
+      } catch (const std::exception &e) {
+        ODR_ERROR(*m_logger, "Exception in HTTP handler: " << e.what());
+      } catch (...) {
+        ODR_ERROR(*m_logger, "Unknown exception in HTTP handler");
+      }
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    });
+
+    m_server->Get("/",
+                  [](const httplib::Request & /*req*/, httplib::Response &res) {
+                    res.set_content("Hello World!", "text/plain");
+                  });
+
+    m_server->Get("/file/" + std::string(prefix_pattern),
+                  [this](const httplib::Request &req, httplib::Response &res) {
+                    if (m_stopping.load(std::memory_order_acquire)) {
+                      res.status = 503;
+                      res.set_content("Service Unavailable", "text/plain");
+                      return;
+                    }
+                    serve_file(req, res);
+                  });
+    m_server->Get("/file/" + std::string(prefix_pattern) + "/(.*)",
+                  [this](const httplib::Request &req, httplib::Response &res) {
+                    if (m_stopping.load(std::memory_order_acquire)) {
+                      res.status = 503;
+                      res.set_content("Service Unavailable", "text/plain");
+                      return;
+                    }
+                    serve_file(req, res);
+                  });
+  }
+
+  ~Impl() {
+    // Ensure the server is properly stopped before destruction.
+    // This prevents crashes from worker threads accessing freed memory.
+    //
+    // IMPORTANT: We must fully stop and destroy the server BEFORE
+    // m_content is destroyed. httplib's thread pool threads may still
+    // be running after stop() returns - they only fully stop when the
+    // Server destructor joins them. By explicitly destroying the server
+    // here (via unique_ptr::reset), we ensure all threads are joined
+    // before any other members are destroyed.
+    m_stopping.store(true, std::memory_order_release);
+    if (m_server) {
+      m_server->stop();
+      m_server.reset(); // Destroy server, join all thread pool threads
+    }
+    // Now safe to let other members destruct - no threads are running
   }
 
+  // Prevent copying - the lambdas capture 'this' so copying would be unsafe
+  Impl(const Impl &) = delete;
+  Impl &operator=(const Impl &) = delete;
+
   const Config &config() const { return m_config; }
 
   const std::shared_ptr<Logger> &logger() const { return m_logger; }
 
   void serve_file(const httplib::Request &req, httplib::Response &res) {
-    std::string id = req.matches[1].str();
-    std::string path = req.matches.size() > 1 ? req.matches[2].str() : "";
-
-    std::unique_lock lock{m_mutex};
-    auto it = m_content.find(id);
-    if (it == m_content.end()) {
-      ODR_ERROR(*m_logger, "Content not found for ID: " << id);
-      res.status = 404;
-      return;
+    try {
+      std::string id = req.matches[1].str();
+      std::string path = req.matches.size() > 1 ? req.matches[2].str() : "";
+
+      std::unique_lock lock{m_mutex};
+      auto it = m_content.find(id);
+      if (it == m_content.end()) {
+        ODR_ERROR(*m_logger, "Content not found for ID: " << id);
+        res.status = 404;
+        return;
+      }
+      auto [_, service] = it->second;
+      lock.unlock();
+
+      serve_file(res, service, path);
+    } catch (const std::exception &e) {
+      ODR_ERROR(*m_logger, "Error handling request: " << e.what());
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    } catch (...) {
+      ODR_ERROR(*m_logger, "Unknown error handling request");
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
     }
-    auto [_, service] = it->second;
-    lock.unlock();
-
-    serve_file(res, service, path);
   }
 
   void serve_file(httplib::Response &res, const HtmlService &service,
@@ -60,17 +125,27 @@ class HttpServer::Impl {
 
     ODR_VERBOSE(*m_logger, "Serving file: " << path);
 
-    httplib::ContentProviderWithoutLength content_provider =
-        [service = service, path = path](const std::size_t offset,
-                                         httplib::DataSink &sink) -> bool {
-      if (offset != 0) {
-        throw std::runtime_error("Invalid offset: " + std::to_string(offset) +
-                                 ". Must be 0.");
-      }
-      service.write(path, sink.os);
-      return false;
-    };
-    res.set_content_provider(service.mimetype(path), content_provider);
+    // Buffer content to avoid streaming issues on Android.
+    // Using ContentProviderWithoutLength (chunked transfer encoding) can cause
+    // SIGSEGV crashes in httplib::Server::write_response_core when:
+    // 1. The client disconnects during transfer
+    // 2. Exceptions are thrown during content generation
+    // 3. The server is stopped while requests are in-flight
+    // By buffering content first, we can handle errors gracefully and use
+    // Content-Length based responses which are more reliable.
+    try {
+      std::ostringstream buffer;
+      service.write(path, buffer);
+      res.set_content(buffer.str(), service.mimetype(path));
+    } catch (const std::exception &e) {
+      ODR_ERROR(*m_logger, "Error serving file " << path << ": " << e.what());
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    } catch (...) {
+      ODR_ERROR(*m_logger, "Unknown error serving file: " << path);
+      res.status = 500;
+      res.set_content("Internal Server Error", "text/plain");
+    }
   }
 
   void connect_service(HtmlService service, const std::string &prefix) {
@@ -88,7 +163,7 @@ class HttpServer::Impl {
   void listen(const std::string &host, const std::uint32_t port) {
     ODR_VERBOSE(*m_logger, "Listening on " << host << ":" << port);
 
-    m_server.listen(host, static_cast<int>(port));
+    m_server->listen(host, static_cast<int>(port));
   }
 
   void clear() {
@@ -107,17 +182,32 @@ class HttpServer::Impl {
   void stop() {
     ODR_VERBOSE(*m_logger, "Stopping HTTP server...");
 
-    clear();
+    // Set stopping flag first to reject new requests immediately.
+    // This prevents new requests from starting while we're shutting down.
+    m_stopping.store(true, std::memory_order_release);
+
+    if (m_server) {
+      // Stop the server to prevent new connections.
+      // Note: httplib::Server::stop() signals shutdown but thread pool
+      // threads may still be running. They only fully stop when the
+      // Server is destroyed. For explicit stop() calls (not destructor),
+      // we destroy the server here to ensure threads are joined.
+      m_server->stop();
+      m_server.reset(); // Destroy server, join all thread pool threads
+    }
 
-    m_server.stop();
+    // Clear content after server is fully destroyed to avoid use-after-free.
+    clear();
   }
 
 private:
   Config m_config;
 
   std::shared_ptr<Logger> m_logger;
 
-  httplib::Server m_server;
+  // Flag to indicate server is shutting down - checked by handlers
+  // to reject new requests during shutdown.
+  std::atomic<bool> m_stopping{false};
 
   struct Content {
     std::string id;
@@ -126,6 +216,14 @@ class HttpServer::Impl {
 
   std::mutex m_mutex;
   std::unordered_map<std::string, Content> m_content;
+
+  // IMPORTANT: m_server is declared LAST and as unique_ptr so we can
+  // explicitly destroy it in the destructor BEFORE other members.
+  // httplib's Server destructor joins thread pool threads, so we must
+  // ensure threads are fully stopped before m_content is destroyed.
+  // Using unique_ptr allows us to call reset() to trigger destruction
+  // at a controlled point in the destructor.
+  std::unique_ptr<httplib::Server> m_server;
 };
 
 HttpServer::HttpServer(const Config &config, std::shared_ptr<Logger> logger)