From e5be0dc35e882b686155e5a484990d8e03286717 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:27:39 +0200 Subject: [PATCH 01/10] refactor: Make HTTPResponse a struct since all fields are public --- src/httpserver.cpp | 30 +++++++++++++++--------------- src/httpserver.h | 12 ++++-------- src/test/httpserver_tests.cpp | 6 +++--- 3 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index fad1db51bec..f6a849f5b2e 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -370,11 +370,11 @@ std::string HTTPHeaders::Stringify() const std::string HTTPResponse::StringifyHeaders() const { return strprintf("HTTP/%d.%d %d %s\r\n%s", - m_version.major, - m_version.minor, - m_status, - HTTPStatusReasonString(m_status), - m_headers.Stringify()); + version.major, + version.minor, + status, + HTTPStatusReasonString(status), + headers.Stringify()); } bool HTTPRequest::LoadControlData(LineReader& reader) @@ -539,13 +539,13 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r HTTPResponse res; // Some response headers are determined in advance and stored in the request - res.m_headers = std::move(m_response_headers); + res.headers = std::move(m_response_headers); // Response version matches request version - res.m_version = m_version; + res.version = m_version; // Add response code - res.m_status = status; + res.status = status; // See libevent evhttp_response_needs_body() // Response headers are different if no body is needed @@ -561,7 +561,7 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r if (m_version.minor == 0) { auto connection_header{m_headers.FindFirst("Connection")}; if (connection_header && ToLower(connection_header.value()) == "keep-alive") { - res.m_headers.Write("Connection", "keep-alive"); + res.headers.Write("Connection", "keep-alive"); keep_alive = true; // HTTP/1.0 connections are closed by default so EOF is sufficient // to indicate end of the body. Adding Content-Length a special case. @@ -572,7 +572,7 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r // HTTP/1.1 if (m_version.minor >= 1) { const int64_t now_seconds{TicksSinceEpoch(NodeClock::now())}; - res.m_headers.Write("Date", FormatRFC1123DateTime(now_seconds)); + res.headers.Write("Date", FormatRFC1123DateTime(now_seconds)); // HTTP/1.1 connections are kept alive by default and always require Content-Length. if (needs_body) needs_content_length = true; @@ -583,20 +583,20 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r } if (needs_content_length) { - res.m_headers.Write("Content-Length", util::ToString(reply_body.size())); + res.headers.Write("Content-Length", util::ToString(reply_body.size())); } - if (needs_body && !res.m_headers.FindFirst("Content-Type")) { + if (needs_body && !res.headers.FindFirst("Content-Type")) { // Default type from libevent evhttp_new_object() - res.m_headers.Write("Content-Type", "text/html; charset=ISO-8859-1"); + res.headers.Write("Content-Type", "text/html; charset=ISO-8859-1"); } auto connection_header{m_headers.FindFirst("Connection")}; if (connection_header && ToLower(connection_header.value()) == "close") { // Might not exist already but we need to replace it, not append to it - res.m_headers.RemoveAll("Connection"); + res.headers.RemoveAll("Connection"); - res.m_headers.Write("Connection", "close"); + res.headers.Write("Connection", "close"); keep_alive = false; } diff --git a/src/httpserver.h b/src/httpserver.h index a0e3c2dcbf6..4e1d1a9a716 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -137,14 +137,10 @@ struct HTTPVersion { /// @} }; - -class HTTPResponse -{ -public: - HTTPVersion m_version; - - HTTPStatusCode m_status{HTTP_INTERNAL_SERVER_ERROR}; - HTTPHeaders m_headers; +struct HTTPResponse { + HTTPVersion version; + HTTPStatusCode status{HTTP_INTERNAL_SERVER_ERROR}; + HTTPHeaders headers; std::string StringifyHeaders() const; }; diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 96d6c387676..2b0d172e1ab 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -188,9 +188,9 @@ BOOST_AUTO_TEST_CASE(http_response_tests) // Response points to headers which already exist because some of them // are set before we even know what the response will be. HTTPResponse res; - res.m_version = {.major = 1, .minor = 1}; - res.m_status = HTTP_OK; - res.m_headers = std::move(headers); + res.version = {.major = 1, .minor = 1}; + res.status = HTTP_OK; + res.headers = std::move(headers); BOOST_CHECK_EQUAL( res.StringifyHeaders(), "HTTP/1.1 200 OK\r\n" From b8cd77237b3425f5a589d7fd24dfb3847c39dcd8 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 17 Aug 2026 22:25:40 +0200 Subject: [PATCH 02/10] refactor: Make HTTPRequest::GetHeader() return saner optional type No need to stick to weird old API from libevent-wrapper days. Makes later commits in the PR cleaner. --- src/httprpc.cpp | 6 +++--- src/httpserver.cpp | 5 ++--- src/httpserver.h | 2 +- src/test/fuzz/http_request.cpp | 10 +++++----- src/test/httpserver_tests.cpp | 16 ++++++++-------- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index ed068a34b8e..f9b95dd8361 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -202,8 +202,8 @@ static void HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req) return; } // Check authorization - std::pair authHeader = req->GetHeader("authorization"); - if (!authHeader.first) { + std::optional auth_header = req->GetHeader("authorization"); + if (!auth_header) { req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA); req->WriteReply(HTTP_UNAUTHORIZED); return; @@ -213,7 +213,7 @@ static void HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req) jreq.context = context; jreq.peerAddr = req->GetPeer().ToStringAddrPort(); jreq.URI = req->GetURI(); - if (!RPCAuthorized(authHeader.second, jreq.authUser)) { + if (!RPCAuthorized(*auth_header, jreq.authUser)) { LogWarning("ThreadRPCServer incorrect password attempt from %s", jreq.peerAddr); /* Deter brute-forcing diff --git a/src/httpserver.cpp b/src/httpserver.cpp index f6a849f5b2e..22dde8a696a 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -693,10 +693,9 @@ std::optional GetQueryParameterFromUri(const std::string_view uri, return std::nullopt; } -std::pair HTTPRequest::GetHeader(const std::string_view hdr) const +std::optional HTTPRequest::GetHeader(const std::string_view hdr) const { - std::optional found{m_headers.FindFirst(hdr)}; - return std::pair{found.has_value(), std::move(found).value_or("")}; + return m_headers.FindFirst(hdr); } void HTTPRequest::WriteHeader(std::string&& hdr, std::string&& value) diff --git a/src/httpserver.h b/src/httpserver.h index 4e1d1a9a716..42d5b155dc0 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -192,7 +192,7 @@ public: CService GetPeer() const; HTTPRequestMethod GetRequestMethod() const { return m_method; } std::optional GetQueryParameter(std::string_view key) const; - std::pair GetHeader(std::string_view hdr) const; + std::optional GetHeader(std::string_view hdr) const; std::string ReadBody() const { return m_body; } void WriteHeader(std::string&& hdr, std::string&& value); diff --git a/src/test/fuzz/http_request.cpp b/src/test/fuzz/http_request.cpp index 75d2729e94e..3ae5355c5ef 100644 --- a/src/test/fuzz/http_request.cpp +++ b/src/test/fuzz/http_request.cpp @@ -51,14 +51,14 @@ FUZZ_TARGET(http_request) // empty string here; LoadBody now populates the body per RFC 9112 framing, so mirror // its branch logic to assert the body matches the framing that produced it. const std::string body = http_request.ReadBody(); - const auto [has_transfer_encoding, transfer_encoding] = http_request.GetHeader("Transfer-Encoding"); - const auto [has_content_length, content_length] = http_request.GetHeader("Content-Length"); - if (has_transfer_encoding && ToLower(transfer_encoding) == "chunked") { + const auto transfer_encoding = http_request.GetHeader("Transfer-Encoding"); + const auto content_length = http_request.GetHeader("Content-Length"); + if (transfer_encoding && ToLower(*transfer_encoding) == "chunked") { // A chunked body is the concatenation of the decoded chunks, bounded by MAX_BODY_SIZE. assert(body.size() <= http_bitcoin::MAX_BODY_SIZE); - } else if (has_content_length) { + } else if (content_length) { // A Content-Length body is exactly that many bytes. - const auto parsed_length{ToIntegral(content_length)}; + const auto parsed_length{ToIntegral(*content_length)}; assert(parsed_length); assert(body.size() == *parsed_length); } else { diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 2b0d172e1ab..94f9920dd87 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -212,11 +212,11 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK_EQUAL(req.GetURI(), "/"); BOOST_CHECK_EQUAL(req.m_version.major, 1); BOOST_CHECK_EQUAL(req.m_version.minor, 1); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Host"), "127.0.0.1"); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Connection"), "close"); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Content-Type"), "application/json"); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Authorization"), "Basic X19jb29raWVfXzo5OGQ5ODQ3MWNmNjg0NzAzYTkzN2EzNzk0ZDFlODQ1NjZmYTRkZjJiMzFkYjhhODI4ZGY4MjVjOTg5ZGI4OTVl"); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Content-Length"), "46"); + BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); + BOOST_CHECK_EQUAL(req.GetHeader("Connection"), "close"); + BOOST_CHECK_EQUAL(req.GetHeader("Content-Type"), "application/json"); + BOOST_CHECK_EQUAL(req.GetHeader("Authorization"), "Basic X19jb29raWVfXzo5OGQ5ODQ3MWNmNjg0NzAzYTkzN2EzNzk0ZDFlODQ1NjZmYTRkZjJiMzFkYjhhODI4ZGY4MjVjOTg5ZGI4OTVl"); + BOOST_CHECK_EQUAL(req.GetHeader("Content-Length"), "46"); BOOST_CHECK_EQUAL(req.m_body.size(), 46); BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount","params":[],"id":1})""\n"); } @@ -317,7 +317,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK_EQUAL(req.m_target, "/"); BOOST_CHECK_EQUAL(req.m_version.major, 1); BOOST_CHECK_EQUAL(req.m_version.minor, 0); - BOOST_CHECK_EQUAL(req.m_headers.FindFirst("Host"), "127.0.0.1"); + BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); // no body is OK BOOST_CHECK_EQUAL(req.m_body.size(), 0); } @@ -448,7 +448,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); // Chunk Trailer was parsed, but ignored BOOST_CHECK_EQUAL(reader.Remaining(), 0); - BOOST_CHECK(!req.GetHeader("Expires").first); + BOOST_CHECK(!req.GetHeader("Expires")); } { // Invalid "chunked" transfer, using roman numerals instead of hex for chunk length @@ -672,7 +672,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); // We read up to the invalid line - BOOST_CHECK_EQUAL(*client->m_req->m_headers.FindFirst("Host"), "127.0.0.1"); + BOOST_CHECK_EQUAL(client->m_req->GetHeader("Host"), "127.0.0.1"); // Buffer was cleared, client should just be disconnected now BOOST_CHECK(client->m_recv_buffer.empty()); From 6fec8d6914bff958d34a5fdad1cee5ff5e9e9355 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:25:31 +0200 Subject: [PATCH 03/10] refactor: Make HTTPRequest fields private Makes sense since they are only set by methods in the class itself, and already had accessors for most fields. --- src/httpserver.cpp | 4 +- src/httpserver.h | 31 ++++++++------- src/test/httpserver_tests.cpp | 73 +++++++++++++++++------------------ 3 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 22dde8a696a..c94863cae33 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1064,8 +1064,8 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req->m_method), - client->m_req->m_target, + RequestMethodString(client->m_req->GetRequestMethod()), + client->m_req->GetURI(), client->m_origin, client->m_id); diff --git a/src/httpserver.h b/src/httpserver.h index 42d5b155dc0..ce2bd4eaac2 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -150,18 +150,6 @@ class HTTPRemoteClient; class HTTPRequest { public: - HTTPRequestMethod m_method; - std::string m_target; - HTTPVersion m_version; - HTTPHeaders m_headers; - std::string m_body; - - //! Pointer to the client that made the request so we know who to respond to. - std::weak_ptr m_client; - - //! Response headers may be set in advance before response body is known - HTTPHeaders m_response_headers; - explicit HTTPRequest(const std::shared_ptr& client) : m_client{client} {} //! Construct with a null client for unit tests explicit HTTPRequest() : m_client{} {} @@ -186,6 +174,9 @@ public: WriteReply(status, std::as_bytes(std::span{reply_body_view})); } + const HTTPVersion& GetVersion() const { return m_version; } + std::shared_ptr GetClient() const { return m_client.lock(); } + // These methods reimplement the API from http_libevent::HTTPRequest // for downstream JSONRPC and REST modules. std::string GetURI() const { return m_target; } @@ -195,6 +186,8 @@ public: std::optional GetHeader(std::string_view hdr) const; std::string ReadBody() const { return m_body; } void WriteHeader(std::string&& hdr, std::string&& value); + std::optional GetChunkSize() const { return m_chunk_size; } + uint64_t GetChunkProgress() const { return m_chunk_read; } enum class State { Init, @@ -206,6 +199,19 @@ public: State GetState() const { return m_state; } void SetState(State state) { m_state = state; } +private: + HTTPRequestMethod m_method; + std::string m_target; + HTTPVersion m_version; + HTTPHeaders m_headers; + std::string m_body; + + //! Pointer to the client that made the request so we know who to respond to. + std::weak_ptr m_client; + + //! Response headers may be set in advance before response body is known + HTTPHeaders m_response_headers; + // If a large request is sent with "Transfer-encoding: chunked" we may // read the chunk size in a separate I/O loop iteration than the chunk // of data itself. Store the chunk size value here until the chunk is read. @@ -214,7 +220,6 @@ public: // Track the progress of the chunk here. uint64_t m_chunk_read{0}; -private: State m_state = State::Init; }; diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 94f9920dd87..5459fb49e29 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -206,19 +206,16 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_method, HTTPRequestMethod::POST); BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::POST); - BOOST_CHECK_EQUAL(req.m_target, "/"); BOOST_CHECK_EQUAL(req.GetURI(), "/"); - BOOST_CHECK_EQUAL(req.m_version.major, 1); - BOOST_CHECK_EQUAL(req.m_version.minor, 1); + BOOST_CHECK_EQUAL(req.GetVersion().major, 1); + BOOST_CHECK_EQUAL(req.GetVersion().minor, 1); BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); BOOST_CHECK_EQUAL(req.GetHeader("Connection"), "close"); BOOST_CHECK_EQUAL(req.GetHeader("Content-Type"), "application/json"); BOOST_CHECK_EQUAL(req.GetHeader("Authorization"), "Basic X19jb29raWVfXzo5OGQ5ODQ3MWNmNjg0NzAzYTkzN2EzNzk0ZDFlODQ1NjZmYTRkZjJiMzFkYjhhODI4ZGY4MjVjOTg5ZGI4OTVl"); BOOST_CHECK_EQUAL(req.GetHeader("Content-Length"), "46"); - BOOST_CHECK_EQUAL(req.m_body.size(), 46); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount","params":[],"id":1})""\n"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount","params":[],"id":1})""\n"); } { // Malformed: no spaces between data @@ -313,13 +310,13 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_method, HTTPRequestMethod::GET); - BOOST_CHECK_EQUAL(req.m_target, "/"); - BOOST_CHECK_EQUAL(req.m_version.major, 1); - BOOST_CHECK_EQUAL(req.m_version.minor, 0); + BOOST_CHECK_EQUAL(req.GetRequestMethod(), HTTPRequestMethod::GET); + BOOST_CHECK_EQUAL(req.GetURI(), "/"); + BOOST_CHECK_EQUAL(req.GetVersion().major, 1); + BOOST_CHECK_EQUAL(req.GetVersion().minor, 0); BOOST_CHECK_EQUAL(req.GetHeader("Host"), "127.0.0.1"); // no body is OK - BOOST_CHECK_EQUAL(req.m_body.size(), 0); + BOOST_CHECK_EQUAL(req.ReadBody(), ""); } { // Malformed: missing colon @@ -345,7 +342,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); // Don't try to read request body if Content-Length is missing - BOOST_CHECK_EQUAL(req.m_body.size(), 0); + BOOST_CHECK_EQUAL(req.ReadBody(), ""); } { // Malformed: Content-Length is not a number @@ -409,7 +406,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})"); } { // Prevent "chunked" transfer from exceeding size limit @@ -445,7 +442,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); BOOST_CHECK(req.LoadBody(reader)); - BOOST_CHECK_EQUAL(req.m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(req.ReadBody(), R"({"method":"getblockcount"})"); // Chunk Trailer was parsed, but ignored BOOST_CHECK_EQUAL(reader.Remaining(), 0); BOOST_CHECK(!req.GetHeader("Expires")); @@ -541,7 +538,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "GET /endpoint HTTP/1.0\n\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "I miss you"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "I miss you"); // Next request sitting in buffer BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); // Complete first request hasn't been moved yet, expect no-op @@ -555,8 +552,8 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) // Read second request client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_target, "/endpoint"); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + BOOST_CHECK_EQUAL(client->m_req->GetURI(), "/endpoint"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); // Buffer is cleared BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); } @@ -579,7 +576,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->receive(std::string(10000, 'x')); BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 10000); client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 10000 * i); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 10000 * i); BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); } BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); @@ -596,7 +593,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "GET /next HTTP/1.0\n\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "body"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "body"); // Only the second request is left over BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 20); } @@ -605,9 +602,9 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) std::shared_ptr client{std::make_shared()}; client->m_req = std::make_unique(client); - BOOST_CHECK(!client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); + BOOST_CHECK(!client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); // First chunk is incomplete @@ -617,40 +614,40 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "10\n" R"({"method)"); client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 16); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 8); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 8); + BOOST_CHECK(client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 16); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 8); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 8); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // More data arrives, chunk is completed. client->receive(R"(":"getbl)""\n"); client->ReadRequest(*client->m_req); // State is reset - BOOST_CHECK(!client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 0); + BOOST_CHECK(!client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); // New data is added to body but body is still incomplete - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 16); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 16); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // Next chunk arrives without terminal CRLF client->receive("a\n" R"(ockcount"})"); client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->m_chunk_size); - BOOST_CHECK_EQUAL(*client->m_req->m_chunk_size, 10); - BOOST_CHECK_EQUAL(client->m_req->m_chunk_read, 10); - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + BOOST_CHECK(client->m_req->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 10); + BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 10); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); // Chunk terminal CRLF arrives with final (size 0) chunk client->receive("\n0\n\n"); client->ReadRequest(*client->m_req); // Body size hasn't changed - BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 26); + BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); // We're done BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), R"({"method":"getblockcount"})"); } { // Invalid headers: error state stops reading @@ -772,7 +769,7 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->receive("\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->m_body, "x"); + BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "x"); } { // Ensure chunk trailer counts towards the headers size limit @@ -868,11 +865,11 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests) // Connected client should have one request already from the static content. if (requests.size() == 1) { // Check the received request - BOOST_CHECK_EQUAL(requests.front()->m_body, R"({"method":"getblockcount","params":[],"id":1})""\n"); + BOOST_CHECK_EQUAL(requests.front()->ReadBody(), R"({"method":"getblockcount","params":[],"id":1})""\n"); BOOST_CHECK_EQUAL(requests.front()->GetPeer().ToStringAddrPort(), "5.5.5.5:6789"); // Inspect the connection pointed to from the request - client = requests.front()->m_client.lock(); + client = requests.front()->GetClient(); BOOST_REQUIRE(client); BOOST_CHECK_EQUAL(client->m_origin, "5.5.5.5:6789"); From 6d9b61d4f8dc39081dc4e0acde36f54cb340807c Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:39:56 +0200 Subject: [PATCH 04/10] refactor: Extract HTTPRemoteClient::MaybeDisconnect() from HTTPServer::DisconnectClients() --- src/httpserver.cpp | 97 +++++++++++++++++++++++++--------------------- src/httpserver.h | 2 + 2 files changed, 54 insertions(+), 45 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index c94863cae33..24b89ce38f2 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1080,57 +1080,64 @@ void HTTPServer::DisconnectClients() const auto now{Now()}; size_t erased = std::erase_if(m_connected, [&](auto& client) { - // First check for idle timeout. We reset the timer when we send and receive data, - // but if the server is busy handling a request we should ignore the timeout until - // the reply is sent. If we did erase the shared_ptr reference in m_connected - // while the server is busy with a request, it might be prematurely dropped before - // the response has been sent, or if the HTTPRequest was holding a temporary shared_ptr - // client on a worker thread - it would keep the socket open even after "disconnecting". - const bool is_idle{m_rpcservertimeout.count() > 0 && - now - client->m_idle_since.load() > m_rpcservertimeout && - !client->m_req_busy}; - - // Disconnect this client due to error, end of communication, or idle timeout. - // May drop unsent data if we are closing due to error. - if (client->m_disconnect || is_idle) { - if (is_idle) { - LogDebug(BCLog::HTTP, - "HTTP client idle timeout %s (id=%llu)", - client->m_origin, - client->m_id); - } - } else { - // Disconnect this client because the server is shutting - // down and we need to disconnect all clients... - if (m_disconnect_all_clients) { - // ...unless we still have data for this client. - if (client->m_connection_busy) { - // There is still data for this healthy-connected client. - // Continue the I/O loop until all data is sent or an error is encountered. - return false; - } else { - // This is a healthy persistent connection (e.g. keep-alive) - // but it's time to say goodbye. - ; - } - } else { - // No reason to disconnect. - return false; - } - } - // No reason NOT to disconnect, log and remove. - LogDebug(BCLog::HTTP, - "Disconnecting HTTP client %s (id=%llu)", - client->m_origin, - client->m_id); - return true; - }); + return client->MaybeDisconnect(now, + m_rpcservertimeout, + /*disconnect_all=*/m_disconnect_all_clients); + }); if (erased > 0) { // Report back to the main thread m_connected_size.fetch_sub(erased, std::memory_order_relaxed); } } +bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all) +{ + // First check for idle timeout. We reset the timer when we send and receive data, + // but if the server is busy handling a request we should ignore the timeout until + // the reply is sent. If we did erase the shared_ptr reference in m_connected + // while the server is busy with a request, it might be prematurely dropped before + // the response has been sent, or if the HTTPRequest was holding a temporary shared_ptr + // client on a worker thread - it would keep the socket open even after "disconnecting". + const bool is_idle{rpcservertimeout.count() > 0 && + now - m_idle_since.load() > rpcservertimeout && + !m_req_busy}; + + // Disconnect this client due to error, end of communication, or idle timeout. + // May drop unsent data if we are closing due to error. + if (m_disconnect || is_idle) { + if (is_idle) { + LogDebug(BCLog::HTTP, + "HTTP client idle timeout %s (id=%llu)", + m_origin, + m_id); + } + } else { + // Disconnect this client because the server is shutting + // down and we need to disconnect all clients... + if (disconnect_all) { + // ...unless we still have data for this client. + if (m_connection_busy) { + // There is still data for this healthy-connected client. + // Continue the I/O loop until all data is sent or an error is encountered. + return false; + } else { + // This is a healthy persistent connection (e.g. keep-alive) + // but it's time to say goodbye. + ; + } + } else { + // No reason to disconnect. + return false; + } + } + // No reason NOT to disconnect, log and remove. + LogDebug(BCLog::HTTP, + "Disconnecting HTTP client %s (id=%llu)", + m_origin, + m_id); + return true; +} + void HTTPServer::ClearConnectedClients() { Assume(!m_thread_socket_handler.joinable()); // must be called after JoinSocketsThreads() diff --git a/src/httpserver.h b/src/httpserver.h index ce2bd4eaac2..af4039cf55e 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -580,6 +580,8 @@ public: HTTPRemoteClient(const HTTPRemoteClient&) = delete; HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; + bool MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all); + /** * Try to read an HTTP request from the receive buffer. * Updates HTTPRequest.m_state and drains buffer on error. From a1183c02aaca32cd391dae863d04a8331a1eefcc Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:45:10 +0200 Subject: [PATCH 05/10] refactor: Extract Send() and Receive() into HTTPRemoteClient from HTTPServer --- src/httpserver.cpp | 109 ++++++++++++++++++++++++--------------------- src/httpserver.h | 3 ++ 2 files changed, 62 insertions(+), 50 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 24b89ce38f2..64e8a2748c6 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -600,10 +600,14 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r keep_alive = false; } - std::shared_ptr client{m_client.lock()}; - if (!client) return; + if (std::shared_ptr client{m_client.lock()}) { + client->Send(res, reply_body, keep_alive); + } +} - client->m_keep_alive = keep_alive; +void HTTPRemoteClient::Send(const HTTPResponse& res, std::span reply_body, bool keep_alive) +{ + m_keep_alive = keep_alive; // Serialize the response headers const std::string headers{res.StringifyHeaders()}; @@ -612,14 +616,14 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r bool send_buffer_was_empty{false}; // Fill the send buffer with the complete serialized response headers + body { - LOCK(client->m_send_mutex); - send_buffer_was_empty = client->m_send_buffer.empty(); - client->m_send_buffer.insert(client->m_send_buffer.end(), headers_bytes.begin(), headers_bytes.end()); + LOCK(m_send_mutex); + send_buffer_was_empty = m_send_buffer.empty(); + m_send_buffer.insert(m_send_buffer.end(), headers_bytes.begin(), headers_bytes.end()); // We've been using std::span up until now but it is finally time to copy // data. The original data will go out of scope when WriteReply() returns. // This is analogous to the memcpy() in libevent's evbuffer_add() - client->m_send_buffer.insert(client->m_send_buffer.end(), reply_body.begin(), reply_body.end()); + m_send_buffer.insert(m_send_buffer.end(), reply_body.begin(), reply_body.end()); // If the buffer already held data, the I/O thread is (or soon will be) // draining it, so flag that there is more data to send. This must happen @@ -629,27 +633,27 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r // between, leaving m_send_ready set on an empty buffer. The I/O loop would // then only ever poll the socket for writeability, never read the client's // next request, and wedge the connection. - if (!send_buffer_was_empty) client->m_send_ready = true; + if (!send_buffer_was_empty) m_send_ready = true; } LogDebug( BCLog::HTTP, "HTTPResponse (status code: %d size: %lld) added to send buffer for client %s (id=%llu)", - status, + res.status, headers_bytes.size() + reply_body.size(), - client->m_origin, - client->m_id); + m_origin, + m_id); // If the send buffer was empty before we wrote this reply, we can try an // optimistic send akin to CConnman::PushMessage() in which we // push the data directly out the socket to client right now, instead // of waiting for the next iteration of the I/O loop. if (send_buffer_was_empty) { - client->MaybeSendBytesFromBuffer(); + MaybeSendBytesFromBuffer(); } // Signal to the I/O loop that we are ready to handle the next request. - client->m_req_busy = false; + m_req_busy = false; } CService HTTPRequest::GetPeer() const @@ -900,43 +904,7 @@ void HTTPServer::SocketHandlerConnected(const IOReadiness& io_readiness) const } if (recv_ready || err_ready) { - char buf[0x10000]; // typical socket buffer is 8K-64K - - const ssize_t nrecv{WITH_LOCK( - client->m_sock_mutex, - return client->m_sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);)}; - - if (nrecv < 0) { - const int err = WSAGetLastError(); - if (IOErrorIsPermanent(err)) { - LogDebug( - BCLog::HTTP, - "Permanent read error from %s (id=%llu): %s", - client->m_origin, - client->m_id, - NetworkErrorString(err)); - client->m_disconnect = true; - } - } else if (nrecv == 0) { - LogDebug( - BCLog::HTTP, - "Received EOF from %s (id=%llu)", - client->m_origin, - client->m_id); - client->m_disconnect = true; - } else { - // Reset idle timeout - client->m_idle_since = Now(); - - // Prevent disconnect until all requests are completely handled. - client->m_connection_busy = true; - - // Copy data from socket buffer to client receive buffer - client->m_recv_buffer.insert( - client->m_recv_buffer.end(), - buf, - buf + nrecv); - } + client->Receive(); } // Process as much received data as we can. // This executes for every client whether or not reading or writing @@ -946,6 +914,47 @@ void HTTPServer::SocketHandlerConnected(const IOReadiness& io_readiness) const } } +void HTTPRemoteClient::Receive() +{ + char buf[0x10000]; // typical socket buffer is 8K-64K + + const ssize_t nrecv{WITH_LOCK( + m_sock_mutex, + return m_sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);)}; + + if (nrecv < 0) { + const int err = WSAGetLastError(); + if (IOErrorIsPermanent(err)) { + LogDebug( + BCLog::HTTP, + "Permanent read error from %s (id=%llu): %s", + m_origin, + m_id, + NetworkErrorString(err)); + m_disconnect = true; + } + } else if (nrecv == 0) { + LogDebug( + BCLog::HTTP, + "Received EOF from %s (id=%llu)", + m_origin, + m_id); + m_disconnect = true; + } else { + // Reset idle timeout + m_idle_since = Now(); + + // Prevent disconnect until all requests are completely handled. + m_connection_busy = true; + + // Copy data from socket buffer to client receive buffer + m_recv_buffer.insert( + m_recv_buffer.end(), + buf, + buf + nrecv); + } +} + void HTTPServer::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) { if (m_stop_accepting) return; diff --git a/src/httpserver.h b/src/httpserver.h index af4039cf55e..33da812c07e 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -580,6 +580,9 @@ public: HTTPRemoteClient(const HTTPRemoteClient&) = delete; HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; + void Send(const HTTPResponse& res, std::span reply_body, bool keep_alive) EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); + void Receive() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); + bool MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all); /** From 5b06d90831691d51e665a017a6da983e0d3920c6 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:21:43 +0200 Subject: [PATCH 06/10] refactor: Replace HTTPServer::MaybeDispatchRequestsFromClient() with HTTPRemoteClient::TryReadRequest() --- src/httpserver.cpp | 19 ++++++++++++------- src/httpserver.h | 18 ++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 64e8a2748c6..b43bdd1dddd 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -910,7 +910,11 @@ void HTTPServer::SocketHandlerConnected(const IOReadiness& io_readiness) const // 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); + if (std::unique_ptr request{HTTPRemoteClient::TryReadRequest(client)}) + { + LOCK(m_request_dispatcher_mutex); + m_request_dispatcher(std::move(request)); + } } } @@ -1029,12 +1033,12 @@ void HTTPServer::ThreadSocketHandler() } } -void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr& client) const +std::unique_ptr HTTPRemoteClient::TryReadRequest(const std::shared_ptr& client) { // 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; + if (client->m_req_busy) return nullptr; if (!client->m_req) { client->m_req = std::make_unique(client); @@ -1053,7 +1057,7 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req, HTTP_CONTENT_TOO_LARGE); client->m_disconnect = true; - return; + return nullptr; } catch (const std::runtime_error& e) { LogDebug( BCLog::HTTP, @@ -1065,7 +1069,7 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_req, HTTP_BAD_REQUEST); client->m_disconnect = true; - return; + return nullptr; } // If the request is ready, hand it to a worker. @@ -1078,10 +1082,11 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptrm_origin, client->m_id); - LOCK(m_request_dispatcher_mutex); client->m_req_busy = true; - m_request_dispatcher(std::move(client->m_req)); + return std::move(client->m_req); } + + return nullptr; } void HTTPServer::DisconnectClients() diff --git a/src/httpserver.h b/src/httpserver.h index 33da812c07e..de4af1d3c7a 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -464,16 +464,6 @@ private: */ void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_request_dispatcher_mutex); - /** - * Try to read HTTPRequests from a client's receive buffer. - * Complete requests are dispatched, incomplete requests are - * left in the buffer to wait for more data. Some read errors - * will mark this client for disconnection. - * @param[in] client The HTTPRemoteClient to read requests from - */ - void MaybeDispatchRequestsFromClient(const std::shared_ptr& client) const - EXCLUSIVE_LOCKS_REQUIRED(!m_request_dispatcher_mutex); - /** * Close underlying socket connections for flagged clients * by removing their shared pointer from m_connected. If an HTTPRemoteClient @@ -585,6 +575,14 @@ public: bool MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all); + /** + * Try to read an HTTPRequest from a client's receive buffer. + * Only complete requests are returned, incomplete requests are + * left in the buffer to wait for more data. Some read errors + * will mark this client for disconnection. + */ + static std::unique_ptr TryReadRequest(const std::shared_ptr& client); + /** * Try to read an HTTP request from the receive buffer. * Updates HTTPRequest.m_state and drains buffer on error. From 10bbae302fb7d659629c93dffc4e9cfde762f78c Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:19:49 +0200 Subject: [PATCH 07/10] refactor: Expose HTTPRemoteClient fields to tests through methods Enables making the fields private later. --- src/httpserver.h | 10 ++ src/test/httpserver_tests.cpp | 248 ++++++++++++++++------------------ 2 files changed, 127 insertions(+), 131 deletions(-) diff --git a/src/httpserver.h b/src/httpserver.h index de4af1d3c7a..f06d300c08a 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -596,6 +596,16 @@ public: * @returns false if we are done with this client and HTTPServer can skip the next read operation from it. */ bool MaybeSendBytesFromBuffer() EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); + + //! Used for tests. + //! @{ + const std::string& GetRecvBuffer() const { return m_recv_buffer; } + const HTTPRequest* GetRequest() const { return m_req.get(); } + //! @} + +protected: + //! Used for tests. + std::string& MutateRecvBuffer() { return m_recv_buffer; } }; /** Initialize HTTP server. diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 5459fb49e29..6d79f0dd32e 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -493,119 +493,114 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) void receive(std::string_view s) { - m_recv_buffer.insert( - m_recv_buffer.end(), - s.begin(), - s.end()); + MutateRecvBuffer().append(s); } }; { // Step through state machine std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); client->receive("POST / HTTP/1.0\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); client->receive("Host: 127.0.0.1\n" "Content-Length: 10\n\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); client->receive("I miss you\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); + auto req{HTTPRemoteClient::TryReadRequest(client)}; + BOOST_REQUIRE(req); + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); } { // Read body over multiple data pushes, multiple requests in same push std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); client->receive("POST / HTTP/1.0\n" "Host: 127.0.0.1\n" "Content-Length: 10\n\n" "I miss"); - client->ReadRequest(*client->m_req); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); // Because of the Content-Length header we know the body is not complete - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Finish sending first request and include second request in the same buffer client->receive(" you" "GET /endpoint HTTP/1.0\n\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "I miss you"); + auto req{HTTPRemoteClient::TryReadRequest(client)}; + BOOST_REQUIRE(req); + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(req->GetURI(), "/"); + BOOST_CHECK(!client->GetRequest()); + BOOST_CHECK_EQUAL(req->ReadBody(), "I miss you"); + req->WriteReply(HTTP_OK, ""); // Mark client as no longer busy // Next request sitting in buffer - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); - // Complete first request hasn't been moved yet, expect no-op - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 24); - - // Reset m_req - client->m_req = std::make_unique(client); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 24); // Read second request - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->GetURI(), "/endpoint"); - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); + req = HTTPRemoteClient::TryReadRequest(client); + BOOST_REQUIRE(req); + BOOST_CHECK(!client->GetRequest()); + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(req->GetURI(), "/endpoint"); + BOOST_CHECK_EQUAL(req->ReadBody().size(), 0); // Buffer is cleared - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 0); } { // A Content-Length body is drained out of the receive buffer as it // arrives, instead of accumulating there until the request is complete. std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); client->receive("POST / HTTP/1.0\n" "Content-Length: 30000\n\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + HTTPRemoteClient::TryReadRequest(client); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Body arrives in 10kB pieces. Each one is copied onto m_body and // erased from the receive buffer, which never holds more than one piece. for (int i = 1; i <= 3; ++i) { client->receive(std::string(10000, 'x')); - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 10000); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 10000 * i); - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 10000); + auto req{HTTPRemoteClient::TryReadRequest(client)}; + if (i < 3) { + BOOST_CHECK(!req.get()); + BOOST_CHECK_EQUAL(client->GetRequest()->ReadBody().size(), 10000 * i); + } else { + BOOST_CHECK(req.get()); + BOOST_CHECK_EQUAL(req->ReadBody().size(), 10000 * i); + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK(!client->GetRequest()); + } + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 0); } - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); } { // A body sent in the same push as the next request is split correctly std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); client->receive("POST / HTTP/1.0\n" "Content-Length: 4\n\n" "body" "GET /next HTTP/1.0\n\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "body"); + auto req{HTTPRemoteClient::TryReadRequest(client)}; + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(req->ReadBody(), "body"); // Only the second request is left over - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 20); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 20); } { // Chunked transfer with state std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - - BOOST_CHECK(!client->m_req->GetChunkSize()); - BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 0); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); // First chunk is incomplete client->receive("GET / HTTP/1.0\n" @@ -613,136 +608,129 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "\n" "10\n" R"({"method)"); - client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->GetChunkSize()); - BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 16); - BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 8); - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 8); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_REQUIRE(client->GetRequest()->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->GetRequest()->GetChunkSize(), 16); + BOOST_CHECK_EQUAL(client->GetRequest()->GetChunkProgress(), 8); + BOOST_CHECK_EQUAL(client->GetRequest()->ReadBody().size(), 8); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // More data arrives, chunk is completed. client->receive(R"(":"getbl)""\n"); - client->ReadRequest(*client->m_req); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); // State is reset - BOOST_CHECK(!client->m_req->GetChunkSize()); - BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 0); + BOOST_CHECK(!client->GetRequest()->GetChunkSize()); + BOOST_CHECK_EQUAL(client->GetRequest()->GetChunkProgress(), 0); // New data is added to body but body is still incomplete - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 16); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK_EQUAL(client->GetRequest()->ReadBody().size(), 16); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Next chunk arrives without terminal CRLF client->receive("a\n" R"(ockcount"})"); - client->ReadRequest(*client->m_req); - BOOST_CHECK(client->m_req->GetChunkSize()); - BOOST_CHECK_EQUAL(*client->m_req->GetChunkSize(), 10); - BOOST_CHECK_EQUAL(client->m_req->GetChunkProgress(), 10); - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK(client->GetRequest()->GetChunkSize()); + BOOST_CHECK_EQUAL(*client->GetRequest()->GetChunkSize(), 10); + BOOST_CHECK_EQUAL(client->GetRequest()->GetChunkProgress(), 10); + BOOST_CHECK_EQUAL(client->GetRequest()->ReadBody().size(), 26); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Chunk terminal CRLF arrives with final (size 0) chunk client->receive("\n0\n\n"); - client->ReadRequest(*client->m_req); + auto req{HTTPRemoteClient::TryReadRequest(client)}; // Body size hasn't changed - BOOST_CHECK_EQUAL(client->m_req->ReadBody().size(), 26); + BOOST_CHECK_EQUAL(req->ReadBody().size(), 26); // We're done - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->ReadBody(), R"({"method":"getblockcount"})"); + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(req->ReadBody(), R"({"method":"getblockcount"})"); } { // Invalid headers: error state stops reading std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); // Request is in the buffer client->receive("POST / HTTP/1.0\n" - "Host: 127.0.0.1\n" - "Invalid header with no colon\n" + "Host: 127.0.0.1\n"); + BOOST_CHECK(!client->GetRecvBuffer().empty()); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); + client->receive("Invalid header with no colon\n" "\n" "body is not read"); - BOOST_CHECK(!client->m_recv_buffer.empty()); - // Reading throws an error, sets state - BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), - std::runtime_error, - HasReason{"HTTP header missing colon (:)"}); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Error); // We read up to the invalid line - BOOST_CHECK_EQUAL(client->m_req->GetHeader("Host"), "127.0.0.1"); + BOOST_CHECK_EQUAL(client->GetRequest()->GetHeader("Host"), "127.0.0.1"); // Buffer was cleared, client should just be disconnected now - BOOST_CHECK(client->m_recv_buffer.empty()); + BOOST_CHECK(client->GetRecvBuffer().empty()); // Even if more data comes in, trying to read again in error state is a no-op client->receive("Content-Length: 2\n\nok"); - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 21); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRecvBuffer().size(), 21); } { // Headers sent in batches that are below MAX_HEADERS_SIZE but the total is excessive std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!client->GetRequest()); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Init); client->receive("POST /huge HTTP/1.0\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); for (int i = 0; i < 410; ++i) { client->receive("key:value\n"); } - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); for (int i = 0; i < 409; ++i) { client->receive("key:value\n"); } - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); // We're at 819 x 10-byte headers // The limit is 8192, three more bytes should throw. client->receive("k:\n"); - BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), - std::runtime_error, - HasReason{"HTTP headers exceed size limit"}); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Error); } { // Client sends chunks that are below the limit but the total is excessive std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Init); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Init); client->receive("POST /huge HTTP/1.0\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsHeaders); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsHeaders); client->receive("Transfer-Encoding: chunked\n\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Send 16-byte chunk client->receive("10\nno auto updates!\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // The next chunk will be of size 32MiB - 16 + 1, below the limit // on its own but not if it were added to the total cumulative body so far. // We don't need to actually send or prepare this amount of data. client->receive("1fffff1\n"); - BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), - http_bitcoin::ContentTooLargeError, - HasReason{"Chunk will exceed max body size"}); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Error); } { // Ensure chunk trailer is parsed over state lines std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); + BOOST_CHECK(!client->GetRequest()); // Send a 1-byte chunk then send the 0-chunk with a trailer but no terminal CRLF client->receive("GET / HTTP/1.0\n" @@ -752,29 +740,29 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "x\n" "0\n" "Digest: sha-4=deadbeef\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Send first part of another trailer line client->receive("Expires:"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Finish the trailer line client->receive("never\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // Terminate client->receive("\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - BOOST_CHECK_EQUAL(client->m_req->ReadBody(), "x"); + auto req{HTTPRemoteClient::TryReadRequest(client)}; + BOOST_CHECK_EQUAL(req->GetState(), HTTPRequest::State::Complete); + BOOST_CHECK_EQUAL(req->ReadBody(), "x"); } { // Ensure chunk trailer counts towards the headers size limit std::shared_ptr client{std::make_shared()}; - client->m_req = std::make_unique(client); + BOOST_CHECK(!client->GetRequest()); client->receive("POST /huge HTTP/1.0\n" "Transfer-Encoding: chunked\n"); // 27 bytes @@ -785,16 +773,14 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) "1\n" "x\n" "0\n"); - client->ReadRequest(*client->m_req); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::NeedsBody); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::NeedsBody); // We're in the trailer section with a total of 8188 bytes of headers. // The limit is 8192, five more bytes should throw. client->receive("k:vv\n"); - BOOST_CHECK_EXCEPTION(client->ReadRequest(*client->m_req), - std::runtime_error, - HasReason{"HTTP headers exceed size limit"}); - BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); + BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(client)); + BOOST_CHECK_EQUAL(client->GetRequest()->GetState(), HTTPRequest::State::Error); } } From d72f67fd6c9db5f0e23c12dde2ac6bf3887a2e17 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:36:47 +0200 Subject: [PATCH 08/10] refactor: Expose additional HTTPRemoteClient fields through accessors --- src/httpserver.cpp | 7 +++---- src/httpserver.h | 7 ++++++- src/test/httpserver_tests.cpp | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index b43bdd1dddd..04839f1406c 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -659,7 +659,7 @@ void HTTPRemoteClient::Send(const HTTPResponse& res, std::span CService HTTPRequest::GetPeer() const { if (std::shared_ptr c{m_client.lock()}) { - return c->m_addr; + return c->GetPeer(); } else { return {}; } @@ -989,7 +989,7 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const for (const auto& http_client : m_connected) { // Safely copy the shared pointer to the socket - std::shared_ptr sock{WITH_LOCK(http_client->m_sock_mutex, return http_client->m_sock;)}; + std::shared_ptr sock{http_client->GetSock()}; // Check if client is ready to send data. Don't try to receive again // until the send buffer is cleared (all data sent to client). @@ -997,8 +997,7 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const // never hold m_sock_mutex and m_send_mutex at the same time here. // MaybeSendBytesFromBuffer() locks m_send_mutex then m_sock_mutex, so nesting // them in the opposite order here would risk a lock-order inversion deadlock. - const bool send_ready{WITH_LOCK(http_client->m_send_mutex, return http_client->m_send_ready;)}; - Sock::Event event = (send_ready ? Sock::SendEvent : Sock::RecvEvent); + Sock::Event event = (http_client->ReadyToSend() ? Sock::SendEvent : Sock::RecvEvent); io_readiness.events_per_sock.emplace(sock, Sock::Events{event}); io_readiness.httpclients_per_sock.emplace(sock, http_client); } diff --git a/src/httpserver.h b/src/httpserver.h index f06d300c08a..83e6882a4a8 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -508,7 +508,7 @@ public: * Written to by http worker threads, read and erased by HTTPServer I/O thread */ /// @{ - Mutex m_send_mutex; + mutable Mutex m_send_mutex; std::vector m_send_buffer GUARDED_BY(m_send_mutex); /// @} @@ -570,6 +570,11 @@ public: HTTPRemoteClient(const HTTPRemoteClient&) = delete; HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; + const std::string& GetOrigin() const { return m_origin; } + const CService& GetPeer() const { return m_addr; } + std::shared_ptr GetSock() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex) { return WITH_LOCK(m_sock_mutex, return m_sock;); } + bool ReadyToSend() const EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex) { return WITH_LOCK(m_send_mutex, return m_send_ready;); } + void Send(const HTTPResponse& res, std::span reply_body, bool keep_alive) EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); void Receive() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index 6d79f0dd32e..c5c5c218c0e 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -857,7 +857,7 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests) // Inspect the connection pointed to from the request client = requests.front()->GetClient(); BOOST_REQUIRE(client); - BOOST_CHECK_EQUAL(client->m_origin, "5.5.5.5:6789"); + BOOST_CHECK_EQUAL(client->GetOrigin(), "5.5.5.5:6789"); // Respond to request requests.front()->WriteReply(HTTP_OK, "874140\n"); From 8f9fd8698a7501370f9d1eada85af65bc32fcc97 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:39:27 +0200 Subject: [PATCH 09/10] refactor: Make HTTPRemoteClient fields private Move-only change. Also makes ReadRequest() private. --- src/httpserver.h | 99 ++++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 49 deletions(-) diff --git a/src/httpserver.h b/src/httpserver.h index 83e6882a4a8..79e530dc914 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -478,6 +478,56 @@ std::optional GetQueryParameterFromUri(std::string_view uri, std::s class HTTPRemoteClient { public: + explicit HTTPRemoteClient(HTTPServer::Id id, const CService& addr, std::unique_ptr socket) + : m_id(id), m_addr(addr), m_origin(addr.ToStringAddrPort()), m_sock{std::move(socket)}, m_idle_since{Now()} {} + + // Disable copies (should only be used as shared pointers) + HTTPRemoteClient(const HTTPRemoteClient&) = delete; + HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; + + const std::string& GetOrigin() const { return m_origin; } + const CService& GetPeer() const { return m_addr; } + std::shared_ptr GetSock() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex) { return WITH_LOCK(m_sock_mutex, return m_sock;); } + bool ReadyToSend() const EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex) { return WITH_LOCK(m_send_mutex, return m_send_ready;); } + + void Send(const HTTPResponse& res, std::span reply_body, bool keep_alive) EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); + void Receive() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); + + bool MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all); + + /** + * Try to read an HTTPRequest from a client's receive buffer. + * Only complete requests are returned, incomplete requests are + * left in the buffer to wait for more data. Some read errors + * will mark this client for disconnection. + */ + static std::unique_ptr TryReadRequest(const std::shared_ptr& client); + + /** + * Push data (if there is any) from client's m_send_buffer to the connected socket. + * @returns false if we are done with this client and HTTPServer can skip the next read operation from it. + */ + bool MaybeSendBytesFromBuffer() EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); + + //! Used for tests. + //! @{ + const std::string& GetRecvBuffer() const { return m_recv_buffer; } + const HTTPRequest* GetRequest() const { return m_req.get(); } + //! @} + +protected: + //! Used for tests. + std::string& MutateRecvBuffer() { return m_recv_buffer; } + +private: + /** + * Try to read an HTTP request from the receive buffer. + * Updates HTTPRequest.m_state and drains buffer on error. + * @param[in] req A HTTPRequest to read into + * @throws std::runtime_error if request is unreadable or violates protocol + */ + void ReadRequest(HTTPRequest& req); + //! ID provided by HTTPServer upon connection and instantiation const HTTPServer::Id m_id; @@ -562,55 +612,6 @@ public: //! Due to optimistic sends it may be updated in either a worker thread or in the //! I/O thread. It is checked in the I/O thread to disconnect idle clients. std::atomic m_idle_since; - - explicit HTTPRemoteClient(HTTPServer::Id id, const CService& addr, std::unique_ptr socket) - : m_id(id), m_addr(addr), m_origin(addr.ToStringAddrPort()), m_sock{std::move(socket)}, m_idle_since{Now()} {} - - // Disable copies (should only be used as shared pointers) - HTTPRemoteClient(const HTTPRemoteClient&) = delete; - HTTPRemoteClient& operator=(const HTTPRemoteClient&) = delete; - - const std::string& GetOrigin() const { return m_origin; } - const CService& GetPeer() const { return m_addr; } - std::shared_ptr GetSock() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex) { return WITH_LOCK(m_sock_mutex, return m_sock;); } - bool ReadyToSend() const EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex) { return WITH_LOCK(m_send_mutex, return m_send_ready;); } - - void Send(const HTTPResponse& res, std::span reply_body, bool keep_alive) EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); - void Receive() EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); - - bool MaybeDisconnect(std::chrono::time_point now, std::chrono::seconds rpcservertimeout, bool disconnect_all); - - /** - * Try to read an HTTPRequest from a client's receive buffer. - * Only complete requests are returned, incomplete requests are - * left in the buffer to wait for more data. Some read errors - * will mark this client for disconnection. - */ - static std::unique_ptr TryReadRequest(const std::shared_ptr& client); - - /** - * Try to read an HTTP request from the receive buffer. - * Updates HTTPRequest.m_state and drains buffer on error. - * @param[in] req A HTTPRequest to read into - * @throws std::runtime_error if request is unreadable or violates protocol - */ - void ReadRequest(HTTPRequest& req); - - /** - * Push data (if there is any) from client's m_send_buffer to the connected socket. - * @returns false if we are done with this client and HTTPServer can skip the next read operation from it. - */ - bool MaybeSendBytesFromBuffer() EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex, !m_sock_mutex); - - //! Used for tests. - //! @{ - const std::string& GetRecvBuffer() const { return m_recv_buffer; } - const HTTPRequest* GetRequest() const { return m_req.get(); } - //! @} - -protected: - //! Used for tests. - std::string& MutateRecvBuffer() { return m_recv_buffer; } }; /** Initialize HTTP server. From 5e0d7a286a49d14018068bd413833ffaef6af37e Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:30:39 +0200 Subject: [PATCH 10/10] refactor: Drastically narrow scope of http_bitcoin namespace and rename it to bitcoin_http http_bitcoin was mostly used during #35182 to distinguish from http_libevent counterpart: - The http_libevent namespace was introduced around the legacy code in 89c54ae4cbc8e58921551d5f1a90eb4683106ccb. - The http_bitcoin namespace was introduced in 68b5d289d19c42de9bebf54a0555053d29721111 and extended in subsequent commits. - The http_libevent namespace together with code it contained was removed in 8c1eea0777c586ce58a500bbea509cd39e4f3507. bitcoin_http is a better name as it is Bitcoin Core's implementation of the HTTP protocol, not HTTP protocol's implementation of bitcoin 402 payment required codes or anything like that. The namespace only remains for a few constants and a type which don't have HTTP in their names. --- src/httprpc.cpp | 1 - src/httpserver.cpp | 11 ++++------- src/httpserver.h | 19 ++++++++----------- src/init.cpp | 4 ---- src/rest.cpp | 1 - src/test/fuzz/http_request.cpp | 7 +++---- src/test/httpserver_tests.cpp | 13 +++---------- 7 files changed, 18 insertions(+), 38 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index f9b95dd8361..0981de5f97b 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -26,7 +26,6 @@ #include #include -using http_bitcoin::HTTPRequest; using util::SplitString; using util::TrimStringView; diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 04839f1406c..0d93fd34cb4 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -50,7 +50,8 @@ static constexpr auto SELECT_TIMEOUT{50ms}; static constexpr int SOCKET_OPTION_TRUE{1}; using common::InvalidPortErrMsg; -using http_bitcoin::HTTPRequest; +using util::LineReader; +using namespace bitcoin_http; struct HTTPPathHandler { @@ -65,7 +66,7 @@ struct HTTPPathHandler /** HTTP module state */ -static std::unique_ptr g_http_server{nullptr}; +static std::unique_ptr g_http_server{nullptr}; //! Handlers for (sub)paths static GlobalMutex g_httppathhandlers_mutex; static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_mutex); @@ -74,7 +75,6 @@ static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_m static ThreadPool g_threadpool_http("http"); static int g_max_queue_depth{100}; -namespace http_bitcoin { /** Check if a network address is allowed to access the HTTP server */ bool HTTPServer::ClientAllowed(const CNetAddr& netaddr) const { @@ -112,7 +112,6 @@ bool HTTPServer::InitHTTPAllowList() LogDebug(BCLog::HTTP, "Allowing HTTP connections from: %s\n", strAllowed); return true; } -} // namespace http_bitcoin /** HTTP request method as string - use for logging only */ std::string_view RequestMethodString(HTTPRequestMethod m) @@ -200,7 +199,7 @@ static void MaybeDispatchRequestToWorker(std::shared_ptr hreq) } } -static void RejectRequest(std::unique_ptr hreq) +static void RejectRequest(std::unique_ptr hreq) { LogDebug(BCLog::HTTP, "Rejecting request while shutting down"); WriteNoStoreErrorReply(*hreq, HTTP_SERVICE_UNAVAILABLE); @@ -261,7 +260,6 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch) } } -namespace http_bitcoin { using util::Split; std::optional HTTPHeaders::FindFirst(const std::string_view key) const @@ -1388,4 +1386,3 @@ void StopHTTPServer() } LogDebug(BCLog::HTTP, "Stopped HTTP server"); } -} // namespace http_bitcoin diff --git a/src/httpserver.h b/src/httpserver.h index 79e530dc914..9a41838102f 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -49,11 +49,10 @@ enum class HTTPRequestMethod { PUT }; -namespace http_bitcoin { - class HTTPRequest; -} +class HTTPRequest; + /** Handler for requests to a certain HTTP path */ -using HTTPRequestHandler = std::function; +using HTTPRequestHandler = std::function; /** Register handler for prefix. * If multiple handlers match a prefix, the first-registered one will @@ -63,9 +62,7 @@ void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPR /** Unregister handler for prefix */ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch); -namespace http_bitcoin { -using util::LineReader; - +namespace bitcoin_http { //! Shortest valid request line, used by libevent in evhttp_parse_request_line() inline constexpr size_t MIN_REQUEST_LINE_LENGTH = std::string_view("GET / HTTP/1.0").size(); @@ -83,6 +80,7 @@ inline constexpr uint64_t MAX_BODY_SIZE{32_MiB}; struct ContentTooLargeError : std::runtime_error { using std::runtime_error::runtime_error; }; +} // namespace bitcoin_http class HTTPHeaders { @@ -163,9 +161,9 @@ public: * @throws std::runtime_error if data is invalid. */ /// @{ - bool LoadControlData(LineReader& reader); - bool LoadHeaders(LineReader& reader); - bool LoadBody(LineReader& reader); + bool LoadControlData(util::LineReader& reader); + bool LoadHeaders(util::LineReader& reader); + bool LoadBody(util::LineReader& reader); /// @} void WriteReply(HTTPStatusCode status, std::span reply_body = {}); @@ -630,6 +628,5 @@ void InterruptHTTPServer(); /** Stop HTTP server */ void StopHTTPServer(); -} // namespace http_bitcoin #endif // BITCOIN_HTTPSERVER_H diff --git a/src/init.cpp b/src/init.cpp index c25e07bf65f..576131c478d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -145,10 +145,6 @@ using common::InvalidPortErrMsg; using common::ResolveErrMsg; -using http_bitcoin::InitHTTPServer; -using http_bitcoin::InterruptHTTPServer; -using http_bitcoin::StartHTTPServer; -using http_bitcoin::StopHTTPServer; using node::ApplyArgsManOptions; using node::BlockManager; using node::CalculateCacheSizes; diff --git a/src/rest.cpp b/src/rest.cpp index fd40d25b9d2..8004b44d062 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -37,7 +37,6 @@ #include -using http_bitcoin::HTTPRequest; using node::GetTransaction; using node::NodeContext; using util::SplitString; diff --git a/src/test/fuzz/http_request.cpp b/src/test/fuzz/http_request.cpp index 3ae5355c5ef..c24413add07 100644 --- a/src/test/fuzz/http_request.cpp +++ b/src/test/fuzz/http_request.cpp @@ -20,9 +20,8 @@ std::string_view RequestMethodString(HTTPRequestMethod m); FUZZ_TARGET(http_request) { - using http_bitcoin::HTTPRequest; - using http_bitcoin::MAX_HEADERS_SIZE; using util::LineReader; + using namespace bitcoin_http; FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; const std::string http_buffer{fuzzed_data_provider.ConsumeRandomLengthString(4096)}; @@ -47,7 +46,7 @@ FUZZ_TARGET(http_request) (void)http_request.GetHeader(header); // Reaching here means LoadControlData/LoadHeaders/LoadBody all succeeded, so the // parsed body must be consistent with the message framing. Before libevent was - // replaced with http_bitcoin::HTTPRequest (#35182), ReadBody() always returned an + // replaced with HTTPRequest (#35182), ReadBody() always returned an // empty string here; LoadBody now populates the body per RFC 9112 framing, so mirror // its branch logic to assert the body matches the framing that produced it. const std::string body = http_request.ReadBody(); @@ -55,7 +54,7 @@ FUZZ_TARGET(http_request) const auto content_length = http_request.GetHeader("Content-Length"); if (transfer_encoding && ToLower(*transfer_encoding) == "chunked") { // A chunked body is the concatenation of the decoded chunks, bounded by MAX_BODY_SIZE. - assert(body.size() <= http_bitcoin::MAX_BODY_SIZE); + assert(body.size() <= MAX_BODY_SIZE); } else if (content_length) { // A Content-Length body is exactly that many bytes. const auto parsed_length{ToIntegral(*content_length)}; diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index c5c5c218c0e..f853a2d01c6 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -12,15 +12,8 @@ #include -using http_bitcoin::GetQueryParameterFromUri; -using http_bitcoin::HTTPHeaders; -using http_bitcoin::HTTPRemoteClient; -using http_bitcoin::HTTPRequest; -using http_bitcoin::HTTPResponse; -using http_bitcoin::HTTPServer; -using http_bitcoin::MAX_BODY_SIZE; -using http_bitcoin::MAX_HEADERS_SIZE; using util::LineReader; +using namespace bitcoin_http; // HTTP request captured from bitcoin-cli constexpr std::string_view full_request = "POST / HTTP/1.1\r\n" @@ -369,7 +362,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) LineReader reader(request, MAX_HEADERS_SIZE); BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); - BOOST_CHECK_EXCEPTION(req.LoadBody(reader), http_bitcoin::ContentTooLargeError, HasReason{"Max body size exceeded"}); + BOOST_CHECK_EXCEPTION(req.LoadBody(reader), ContentTooLargeError, HasReason{"Max body size exceeded"}); } { // Content-Length exactly on the limit @@ -423,7 +416,7 @@ BOOST_AUTO_TEST_CASE(http_request_tests) LineReader reader(excessive_chunk_size, MAX_HEADERS_SIZE); BOOST_CHECK(req.LoadControlData(reader)); BOOST_CHECK(req.LoadHeaders(reader)); - BOOST_CHECK_EXCEPTION(req.LoadBody(reader), http_bitcoin::ContentTooLargeError, HasReason{"Chunk will exceed max body size"}); + BOOST_CHECK_EXCEPTION(req.LoadBody(reader), ContentTooLargeError, HasReason{"Chunk will exceed max body size"}); } { // Allow (but ignore) Chunk Extensions