From 7ee7df988ef80d1b027ea21cc27e3c86da04aa55 Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@pm.me>
Date: Thu, 3 Apr 2025 12:28:42 -0400
Subject: [PATCH] HTTPServer: use a queue to pipeline requests from each
 connected client

See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2

> A server MAY process a sequence of pipelined requests in
  parallel if they all have safe methods (Section 4.2.1 of [RFC7231]),
  but it MUST send the corresponding responses in the same order that
  the requests were received.

We choose NOT to process requests in parallel. They are executed in
the order recevied as well as responded to in the order received.
This prevents race conditions where old state may get sent in response
to requests that are very quick to process but were requested later on
in the queue.
---
 src/httpserver.cpp | 26 ++++++++++++++++++++++----
 src/httpserver.h   | 11 +++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index a5d56be0fb6..ad6d020d3a3 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1072,6 +1072,9 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> r
         // in the next loop iteration.
         m_client->m_send_ready = true;
     }
+
+    // Signal to the I/O loop that we are ready to handle the next request.
+    m_client->m_req_busy = false;
 }
 
 util::Expected<void, std::string> HTTPServer::BindAndStartListening(const CService& to)
@@ -1293,10 +1296,13 @@ void HTTPServer::SocketHandlerConnected(const IOReadiness& io_readiness) const
                     client->m_recv_buffer.end(),
                     buf,
                     buf + nrecv);
-                // Process as much received data as we can
-                MaybeDispatchRequestsFromClient(client);
             }
         }
+        // Process as much received data as we can.
+        // This executes for every client whether or not reading or writing
+        // took place because it also (might) parse a request we have already
+        // received and pass it to a worker thread.
+        MaybeDispatchRequestsFromClient(client);
     }
 }
 
@@ -1410,8 +1416,20 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemot
             client->m_origin,
             client->m_id);
 
-        // handle request
-        m_request_dispatcher(std::move(req));
+        // add request to client queue
+        client->m_req_queue.push_back(std::move(req));
+    }
+
+    // If we are already handling a request from
+    // this client, do nothing. We'll check again on the next I/O
+    // 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()) {
+        client->m_req_busy = true;
+        m_request_dispatcher(std::move(client->m_req_queue.front()));
+        client->m_req_queue.pop_front();
     }
 }
 
diff --git a/src/httpserver.h b/src/httpserver.h
index cbb9dd43298..339cd45f222 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -6,6 +6,7 @@
 #define BITCOIN_HTTPSERVER_H
 
 #include <atomic>
+#include <deque>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -538,6 +539,16 @@ public:
      */
     std::vector<std::byte> 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;
+
+    //! 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.
+    std::atomic_bool m_req_busy{false};
+
     /**
      * Response data destined for this client.
      * Written to by http worker threads, read and erased by HTTPServer I/O thread