http: only read one HTTPRequest at a time per client

src/httpserver.cpp

@@ -30,7 +30,6 @@
 #include <condition_variable>
 #include <cstdio>
 #include <cstdlib>
-#include <deque>
 #include <memory>
 #include <optional>
 #include <span>
@@ -996,13 +995,13 @@ void HTTPServer::ThreadSocketHandler()
 
 void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemoteClient>& client) const
 {
-    // Try reading (potentially multiple) HTTP requests from the buffer
-    while (!client->m_recv_buffer.empty()) {
+    // Try reading the next HTTP request from the buffer
+    if (!client->m_req) {
         // Create a new request object and try to fill it with data from the receive buffer
         auto req = std::make_unique<HTTPRequest>(client);
         try {
             // Stop reading if we need more data from the client to parse a complete request
-            if (!client->ReadRequest(*req)) break;
+            if (!client->ReadRequest(*req)) return;
         } catch (const ContentTooLargeError& e) {
             LogDebug(
                 BCLog::HTTP,
@@ -1037,8 +1036,8 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemot
             client->m_origin,
             client->m_id);
 
-        // add request to client queue
-        client->m_req_queue.push_back(std::move(req));
+        // Move request to client
+        client->m_req = std::move(req);
     }
 
     // If we are already handling a request from
@@ -1046,12 +1045,11 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemot
     // loop iteration.
     if (client->m_req_busy) return;
 
-    // Otherwise, if there is a pending request in the queue, handle it.
-    if (!client->m_req_queue.empty()) {
+    // Otherwise, if there is a request ready to go, handle it.
+    if (client->m_req) {
         LOCK(m_request_dispatcher_mutex);
         client->m_req_busy = true;
-        m_request_dispatcher(std::move(client->m_req_queue.front()));
-        client->m_req_queue.pop_front();
+        m_request_dispatcher(std::move(client->m_req));
     }
 }
 
@@ -1102,6 +1100,7 @@ void HTTPServer::DisconnectClients()
                                                  "Disconnecting HTTP client %s (id=%llu)",
                                                  client->m_origin,
                                                  client->m_id);
+                                        client->ReleaseRequest();
                                         return true;
                                     });
     if (erased > 0) {
@@ -1116,6 +1115,9 @@ void HTTPServer::ClearConnectedClients()
     if (m_connected.empty()) return;
     LogWarning("Force-disconnecting %d HTTP client(s) that did not disconnect gracefully", m_connected.size());
     m_connected_size.fetch_sub(m_connected.size(), std::memory_order_relaxed);
+    for (auto& client : m_connected) {
+        client->ReleaseRequest();
+    }
     m_connected.clear();
 }
 

src/httpserver.h

@@ -6,7 +6,6 @@
 #define BITCOIN_HTTPSERVER_H
 
 #include <atomic>
-#include <deque>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -479,10 +478,9 @@ public:
     std::string m_recv_buffer{};
 
     //! Requests from a client must be processed in the order in which
-    //! they were received, blocking on a per-client basis. We won't
-    //! process the next request in the queue if we are currently busy
-    //! handling a previous request.
-    std::deque<std::unique_ptr<HTTPRequest>> m_req_queue;
+    //! they were received, blocking on a per-client basis. We read
+    //! one request at a time from the socket buffer then pass it to a worker.
+    std::unique_ptr<HTTPRequest> m_req;
 
     //! Set to true by the I/O thread when a request is popped off
     //! and passed to a worker thread, reset to false by the worker thread.
@@ -555,6 +553,12 @@ public:
     HTTPRemoteClient(const HTTPRemoteClient&) = delete;
     HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete;
 
+    //! Release any in-progress request. HTTPRequest holds a shared_ptr back to its
+    //! HTTPRemoteClient to keep the client alive from a worker thread. If a request
+    //! hasn't been moved to a worker yet it will prevent the client from destructing
+    //! and never close the socket. Therefore this must be called when disconnecting.
+    void ReleaseRequest() { m_req.reset(); }
+
     /**
      * Try to read an HTTP request from the receive buffer.
      * @param[in]   req     A HTTPRequest to read into