http: update m_keep_alive under m_send_mutex #36259

pull azuchi wants to merge 1 commits into bitcoin:master from azuchi:http-keepalive-under-send-mutex changing 3 files +156 −3

azuchi commented at 9:07 AM on September 15, 2026: contributor
HTTPRemoteClient::Send() sets m_keep_alive before taking m_send_mutex and appending the response to the send buffer, while the I/O thread reads it under m_send_mutex in MaybeSendBytesFromBuffer() right after the buffer drains to empty. The two updates are therefore not ordered with respect to each other, and a pipelining client can lose a response:
1. Response 1 (keep-alive) is partially sent and the I/O thread is still draining it.
2. Request 2 (Connection: close) is parsed from the receive buffer and dispatched to a worker (the send throttle from #36174 only holds a request back once the buffer exceeds MAX_BODY_SIZE).
3. The worker clears m_keep_alive; the I/O thread then finishes sending response 1, sees an empty buffer with m_keep_alive == false, and sets m_disconnect.
4. The worker appends response 2, which is dropped when the client is disconnected on the next loop iteration.
This PR moves the assignment into the critical section that appends the response, and turns the field from std::atomic_bool into a plain bool GUARDED_BY(m_send_mutex) so Clang's thread-safety analysis enforces the ordering (the only reader already holds the mutex). With the flag and the buffer updated together, the I/O thread can no longer observe "empty && !keep_alive" between two pipelined responses.

The ordering dates back to the initial HTTP server implementation (#35182); #35829/#36007 only moved the code. The window is small and bitcoin-cli does not pipeline, so this is a low-severity fix.

Test

http_pipelined_keepalive_close_tests reproduces the interleaving deterministically with a mock socket: Send() fails with EAGAIN until the second request has been dispatched, then the I/O thread's next Send() (made while holding m_send_mutex) signals the worker to write its reply, waits for it to reach HTTPRemoteClient::Send(), and only then flushes the first reply. Sends are refused again until the test releases them, so the second reply is only delivered if the I/O loop kept the connection open. Without the first commit the test fails with server.GetConnectionsCount() == 1 has failed [0 != 1]: the client was disconnected with the second reply still in its send buffer.

http: update m_keep_alive under m_send_mutex

HTTPRemoteClient::Send() wrote m_keep_alive before taking m_send_mutex
and appending the response to m_send_buffer. The I/O thread decides in
MaybeSendBytesFromBuffer() (under m_send_mutex) whether to disconnect
by checking "buffer drained to empty && !m_keep_alive". Because the flag
and the buffer were not updated together, a pipelined client could lose
a response:

 1. Response 1 (keep-alive) is partially sent; the I/O thread is still
    draining it.
 2. Request 2 ("Connection: close") is parsed from the receive buffer
    and dispatched to a worker (the send throttle only holds requests
    back once the buffer exceeds MAX_BODY_SIZE).
 3. The worker clears m_keep_alive, then the I/O thread finishes
    sending response 1, observes an empty buffer with
    m_keep_alive == false and sets m_disconnect.
 4. The worker appends response 2, which is dropped when the client is
    disconnected on the next I/O loop iteration.

Move the assignment into the same critical section that appends the
response, and change the field from std::atomic_bool to a plain bool
annotated GUARDED_BY(m_send_mutex) so that Clang's thread-safety
analysis enforces the ordering. The only reader already holds
m_send_mutex.

Add a unit test that reproduces the interleaving deterministically with a
mock socket: it holds the first reply in the send buffer until the second
(pipelined, "Connection: close") request has been dispatched, then lets
the I/O thread flush it while the worker is about to write the second
reply. Without the fix the client is disconnected with the second reply
still in the buffer.

dc8fb12509

DrahtBot added the label RPC/REST/ZMQ on Sep 15, 2026
DrahtBot commented at 9:07 AM on September 15, 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/36259.



Reviews

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

Type Reviewers

Concept ACK winterrdog

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
winterrdog commented at 1:23 PM on September 15, 2026: contributor

concept ACK

makes sense to update m_keep_alive in the same critical section as the send buffer, since both are read together under the same lock (m_send_mutex). this keeps the related state changes in sync and does not allow the I/O thread to view an inconsistent state while the next response is being queued

Type	Reviewers
Concept ACK	winterrdog

Contributors

Labels

Linked (view graph)

#35182 Replace libevent with our own HTTP and socket-handling implementation #35829 http: Make class fields private and make HTTPResponse a struct #36007 http: Make HTTPRequest::m_client a weak_ptr #36174 http: throttle send buffer when client stops draining

http: update m_keep_alive under m_send_mutex #36259

Test

Code Coverage & Benchmarks

Reviews