Add state to HTTPRequest #35735

pinheadmz commented at 10:22 AM on July 16, 2026: member

This PR reduces the memory consumption of the HTTP Server when reading data from connected clients, and improves performance especially when requests are large (i.e. requiring multiple TCP packets).

In #35182 the server copies as much data as it can from the socket into application memory, and then tries to parse as many complete HTTP requests as possible from that data. If a request is discovered to be incomplete, the in-progress request is abandoned. The server tries again on the next I/O cycle to read the same data from the buffer, duplicating work as many times as it takes before the client finishes sending the request (or times out).

This PR implements two improvements to this:

Only parse one request at a time from the receive buffer. The server processes requests from each client in series anyway.
Add state to HTTPRequest so it can be filled with data from the receive buffer over multiple I/O loop iterations without losing progress.

If a client sends large or multiple requests, that data will sit in the kernel's socket buffer instead of the application memory. Eventually the socket buffer will fill up and TCP backpressure will kick in, dropping the TCP window to 0 and blocking the client from sending any more.

A state machine for HTTPRemoteClient was discussed previously to control resource consumption. Another nice benefit of this model (for a follow-up PR) will be to insert the RPC authentication check after reading 8kB-limited headers but before the 32MB-limited request body.

DrahtBot commented at 10:22 AM on July 16, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35735.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	brunoerg, janb84, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

chunk_sized -> chunk-sized [typo in comment; the intended phrase is clearer with the hyphen]

2026-07-21 19:38:08

brunoerg commented at 11:35 AM on July 16, 2026: contributor

Concept ACK

janb84 commented at 4:51 PM on July 16, 2026: contributor

big concept ACK fb0a085cc9152725b8152975c2fb8a0898f6939b

w0xlt commented at 5:50 PM on July 16, 2026: contributor

Concept ACK

in src/httpserver.cpp:480 in fb0a085cc9

 511 |  
 512 | -            // Pack chunk onto body
 513 | -            m_body += reader.ReadLength(*chunk_size);
 514 | +            // Pack chunk onto body and clear state
 515 | +            m_body += reader.ReadLength(*m_chunk_size);
 516 | +            m_chunk_size.reset();

brunoerg commented at 6:40 PM on July 16, 2026:

fb0a085cc9152725b8152975c2fb8a0898f6939b: A mix of my head and claude: I think that here might be wrong because It resets m_chunk_size before confirming it actually read the trailing CRLF that terminates a chunk.

brunoerg commented at 6:47 PM on July 16, 2026:

It's interesting that the fuzz target would not catch it because it feeds the whole buffer in one shot.

pinheadmz commented at 2:28 PM on July 21, 2026:

Good catch here and actually indicates a bit more logic was needed on chunk processing. What I had before wouldn't read an incomplete chunk from the buffer (it would wait until the entire chunk was in the buffer). I updated it to read partial chunks and just track the amount of data read for an in progress chunk as part of the state. Then reading the terminal CRLF is done after a chunk is complete, no matter how many calls to LoadBody() it needed. Chunk state is only cleared after that.

I also added a state-clearing after the final chunk (with size 0) is read. It shouldn't be needed any more there but I felt it indicates to the code reviewer a bit more where the ends of the logic branches are.

in test/functional/interface_http.py:240 in fb0a085cc9

 244 | -        # detecting the broken socket (which it may still be trying to write to).
 245 | +        # Split off the send into a background thread. When the server detects
 246 | +        # the excessive size it will stop reading from the socket, but the client
 247 | +        # will continue trying to write until the backpressure eventually
 248 | +        # drops the TCP window size to 0. While the send operation is blocking until
 249 | +        # it times out, we can still receive the server's reponse in the foreground.

winterrdog commented at 10:12 PM on July 18, 2026:

nit: small typo on response

        # it times out, we can still receive the server's response in the foreground.

pinheadmz commented at 7:21 PM on July 21, 2026:

👍

in test/functional/interface_http.py:346 in fb0a085cc9

 354 | -        # detecting the broken socket (which it may still be trying to write to).
 355 | +        # Split off the send into a background thread. When the server detects
 356 | +        # the excessive size it will stop reading from the socket, but the client
 357 | +        # will continue trying to write until the backpressure eventually
 358 | +        # drops the TCP window size to 0. While the send operation is blocking until
 359 | +        # it times out, we can still receive the server's reponse in the foreground.

winterrdog commented at 10:13 PM on July 18, 2026:

nit: same as before, just a small typo on response

        # it times out, we can still receive the server's response in the foreground.

pinheadmz commented at 7:21 PM on July 21, 2026:

👍

in src/httpserver.cpp:1115 in fb0a085cc9

1114 | @@ -1106,22 +1115,41 @@ void HTTPServer::ClearConnectedClients()
1115 |      m_connected.clear();

winterrdog commented at 12:01 AM on July 19, 2026:

this is probably outside the scope of this PR, but it felt close enough to the surrounding changes that it seemed worth mentioning

just like you did in DisconnectClients, i wonder if it also belongs here. it looks like clients still sitting in m_connected when ClearConnectedClients() runs can keep themselves alive through HTTPRequest::m_client, preventing the HTTPRemoteClient (and its socket) from ever being destroyed

my reasoning is roughly: HTTPRequest holds a shared_ptr<HTTPRemoteClient> back to its owning client, while client->m_req owns the HTTPRequest. as long as m_req is non-null, the client effectively holds an extra reference to itself. m_connected.clear() only drops the references owned by m_connected. if m_req is still set, the reference count never reaches zero, so the destructor never runs. ClearConnectedClients() only deals with the stragglers that made it past DisconnectClients(), so these seem like the clients most likely to still have a live m_req. by this point the HTTP thread pool has already been stopped, so i do not see another owner that would eventually break the cycle

sth along these lines is what i had in mind:

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index c3a541f8c1..0b506a3895 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -1111,6 +1111,7 @@ void HTTPServer::ClearConnectedClients()
     Assume(!m_thread_socket_handler.joinable()); // must be called after JoinSocketsThreads()
     if (m_connected.empty()) return;
     LogWarning("Force-disconnecting %d HTTP client(s) that did not disconnect gracefully", m_connected.size());
+    for (const auto& client : m_connected) client->m_req.reset();
     m_connected_size.fetch_sub(m_connected.size(), std::memory_order_relaxed);
     m_connected.clear();
 }

i could well be missing another cleanup path. does anything else clear m_req for these clients before this runs, or is this a real leak ?

pinheadmz commented at 7:09 PM on July 21, 2026:

Thanks I think this is a real leak, although it would only leak during shutdown. Still, we should clean it up. I added a helper method to HTTPRemoteClient and called it from both disconnect sites.

http: only read one HTTPRequest at a time per client 5820294a92

Add state to HTTPRequest to avoid duplicate work over I/O cycles 17eecd72f1

pinheadmz force-pushed on Jul 21, 2026

pinheadmz commented at 7:29 PM on July 21, 2026: member

push to ce01fddeb1caafaec2bb2e7157cd607d4a350620

Addressed review feedback and added another commit with unit test coverage over the HTTPRequest state machine.

Major behavioral changes:

Read incomplete chunked-encoding chunks from the buffer and don't reset state until the chunk-terminal CRLF is parsed
Catch errors in ReadRequest, set error state and clear memory there before continuing (by throwing)

test: cover HTTPRequest state machine d359b053e1

pinheadmz force-pushed on Jul 21, 2026

DrahtBot added the label CI failed on Jul 21, 2026

DrahtBot commented at 7:38 PM on July 21, 2026: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/29861699031/job/88739755985 LLM reason (✨ experimental): CI failed because ctest/AddressSanitizer reported memory leaks during httpserver_tests (test 251), causing the docker exec ... 03_test_script.sh step to exit non-zero.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pinheadmz commented at 7:38 PM on July 21, 2026: member

push to d359b053e189ce85b5f54929334ef94b4a3f370f

hot fix to the new unit tests which were leaking for the exact reason mentioned by @winterrdog in #35735 (review)

DrahtBot removed the label CI failed on Jul 21, 2026