highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-09-11 21:20:39 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#36007: http: Make HTTPRequest::m_client a weak_ptr

Browse Source 
979a42ec17 http: Make HTTPRequest::m_client a weak_ptr (Hodlinator)

Pull request description:

  Removes the need for `HTTPClient::ReleaseRequest()` as the client<->request cycle is broken. Not having to remember to call `ReleaseRequest()` reduces cognitive load.

  Follow-up to #35735.

ACKs for top commit:
  pinheadmz:
    untested ACK 979a42ec17

Tree-SHA512: b740a765ffe0592055819e71df8654614e9e140bb77e4a6d146045255f1db9b470ae4a1a77aa16a0d3f3519b7c0d1e9f8c9fbc4c29aab28a2d55ea828ca912c6
This commit is contained in:
merge-script
2026-08-18 13:12:57 +01:00
parent 82b3bfe38c 979a42ec17
commit 381c331219
3 changed files with 22 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/httpserver.cpp
View File

@@ -600,7 +600,10 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> 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<const std::byte> 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<const std::byte> 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<const std::byte> 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<std::string> 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();
}

10
src/httpserver.h
View File

@@ -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<HTTPRemoteClient> m_client;
std::weak_ptr<HTTPRemoteClient> m_client;
//! Response headers may be set in advance before response body is known
HTTPHeaders m_response_headers;
explicit HTTPRequest(std::shared_ptr<HTTPRemoteClient> client) : m_client{std::move(client)} {}
explicit HTTPRequest(const std::shared_ptr<HTTPRemoteClient>& 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.

25
src/test/httpserver_tests.cpp
View File

@@ -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