From 979a42ec17369b55a3a20c9e45965e6733fbeb19 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Tue, 18 Aug 2026 09:44:03 +0200 Subject: [PATCH] http: Make HTTPRequest::m_client a weak_ptr This removes the need for HTTPClient::ReleaseRequest() as the client<->request cycle is broken. Not having to remember to call ReleaseRequest() reduces cognitive load. --- src/httpserver.cpp | 33 ++++++++++++++++++--------------- src/httpserver.h | 10 ++-------- src/test/httpserver_tests.cpp | 25 ++----------------------- 3 files changed, 22 insertions(+), 46 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index d72e9d9fd7e..f78e507112c 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -600,7 +600,10 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r keep_alive = false; } - m_client->m_keep_alive = keep_alive; + std::shared_ptr client{m_client.lock()}; + if (!client) return; + + client->m_keep_alive = keep_alive; // Serialize the response headers const std::string headers{res.StringifyHeaders()}; @@ -609,14 +612,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(m_client->m_send_mutex); - send_buffer_was_empty = m_client->m_send_buffer.empty(); - m_client->m_send_buffer.insert(m_client->m_send_buffer.end(), headers_bytes.begin(), headers_bytes.end()); + 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()); // 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() - m_client->m_send_buffer.insert(m_client->m_send_buffer.end(), reply_body.begin(), reply_body.end()); + client->m_send_buffer.insert(client->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 @@ -626,7 +629,7 @@ 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) m_client->m_send_ready = true; + if (!send_buffer_was_empty) client->m_send_ready = true; } LogDebug( @@ -634,24 +637,28 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span r "HTTPResponse (status code: %d size: %lld) added to send buffer for client %s (id=%llu)", status, headers_bytes.size() + reply_body.size(), - m_client->m_origin, - m_client->m_id); + client->m_origin, + client->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) { - m_client->MaybeSendBytesFromBuffer(); + client->MaybeSendBytesFromBuffer(); } // Signal to the I/O loop that we are ready to handle the next request. - m_client->m_req_busy = false; + client->m_req_busy = false; } CService HTTPRequest::GetPeer() const { - return m_client->m_addr; + if (std::shared_ptr c{m_client.lock()}) { + return c->m_addr; + } else { + return {}; + } } std::optional HTTPRequest::GetQueryParameter(const std::string_view key) const @@ -1116,7 +1123,6 @@ void HTTPServer::DisconnectClients() "Disconnecting HTTP client %s (id=%llu)", client->m_origin, client->m_id); - client->ReleaseRequest(); return true; }); if (erased > 0) { @@ -1131,9 +1137,6 @@ 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(); } diff --git a/src/httpserver.h b/src/httpserver.h index c90830eb063..6730d3bb2d1 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -161,12 +161,12 @@ public: std::string m_body; //! Pointer to the client that made the request so we know who to respond to. - std::shared_ptr m_client; + std::weak_ptr m_client; //! Response headers may be set in advance before response body is known HTTPHeaders m_response_headers; - explicit HTTPRequest(std::shared_ptr client) : m_client{std::move(client)} {} + explicit HTTPRequest(const std::shared_ptr& client) : m_client{client} {} //! Construct with a null client for unit tests explicit HTTPRequest() : m_client{} {} @@ -579,12 +579,6 @@ 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. * Updates HTTPRequest.m_state and drains buffer on error. diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index f05a1857b31..96d6c387676 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -521,10 +521,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->receive("I miss you\n"); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - - // m_req holds a shared_ptr back to the client, so break the cycle - // before the client goes out of scope (as the server does on disconnect). - client->ReleaseRequest(); } { // Read body over multiple data pushes, multiple requests in same push @@ -563,8 +559,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) BOOST_CHECK_EQUAL(client->m_req->m_body.size(), 0); // Buffer is cleared BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); - - client->ReleaseRequest(); } { // A Content-Length body is drained out of the receive buffer as it @@ -589,8 +583,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 0); } BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); - - client->ReleaseRequest(); } { // A body sent in the same push as the next request is split correctly @@ -607,8 +599,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) BOOST_CHECK_EQUAL(client->m_req->m_body, "body"); // Only the second request is left over BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 20); - - client->ReleaseRequest(); } { // Chunked transfer with state @@ -661,8 +651,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) // We're done BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); BOOST_CHECK_EQUAL(client->m_req->m_body, R"({"method":"getblockcount"})"); - - client->ReleaseRequest(); } { // Invalid headers: error state stops reading @@ -693,8 +681,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_recv_buffer.size(), 21); - - client->ReleaseRequest(); } { // Headers sent in batches that are below MAX_HEADERS_SIZE but the total is excessive @@ -726,8 +712,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) std::runtime_error, HasReason{"HTTP headers exceed size limit"}); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); - - client->ReleaseRequest(); } { // Client sends chunks that are below the limit but the total is excessive @@ -757,8 +741,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) http_bitcoin::ContentTooLargeError, HasReason{"Chunk will exceed max body size"}); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); - - client->ReleaseRequest(); } { // Ensure chunk trailer is parsed over state lines @@ -791,8 +773,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) client->ReadRequest(*client->m_req); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Complete); BOOST_CHECK_EQUAL(client->m_req->m_body, "x"); - - client->ReleaseRequest(); } { // Ensure chunk trailer counts towards the headers size limit @@ -818,8 +798,6 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) std::runtime_error, HasReason{"HTTP headers exceed size limit"}); BOOST_CHECK_EQUAL(client->m_req->GetState(), HTTPRequest::State::Error); - - client->ReleaseRequest(); } } @@ -894,7 +872,8 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests) 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; + client = requests.front()->m_client.lock(); + BOOST_REQUIRE(client); BOOST_CHECK_EQUAL(client->m_origin, "5.5.5.5:6789"); // Respond to request