http: don't register RecvEvent while a request is in-flight #36276

pinheadmz commented at 3:34 PM on September 16, 2026: member

Follow up to #36123, which implemented a read-side throttle so the server won't drain the socket buffer into application memory without bound. There was a tiny window left unaddressed which allowed one additional read operation of up to 65kB from the socket buffer in rare cases, correctly failing the test.

The original throttle works by only registering socket recv events when EITHER:

A pending request is being built from socket data (m_req != nullptr)
The receive buffer m_recv_buffer is empty.

The idea is "don't read from the socket unless we need to, in order to complete an incoming request".

The new rule applied in this PR is "if a request is already busy in a worker thread, don't read at all".

The test sends a blocking HTTP request followed by a flood of data, expecting that flood to be blocked at some point within a few seconds by TCP backpressure: the application stops reading from the socket and then the kernel stops reading from the client (BlockingIOError in the python test).

The TCP spec requires clients to periodically probe the server to learn when the window has been reopened: https://www.rfc-editor.org/rfc/rfc9293.html#name-zero-window-probing

Because of the bug in the throttle logic from #36123, this probe would discover a non-zero window and send more data, resetting the stall timer in the test. This may repeat until the socket buffer is full again (the kernel replaced the 65kB the application foolishly drained) at which point the stream is truly blocked.

On linux platforms this probe is sent around 200ms, so even after a few cycles the server throttle would still register as solid to the test.

On macos however, given the small size of the open window, the probe is delayed 5 seconds. The cycle repeating every 5 seconds breaks the assumptions in the test and is interpreted as no throttle at all.

Note that the worst-case scenario for this bug is only 65kB so this is not a critical OOM fix.

I spent a few days with GPT, kimi, Sonnet and Opus trying to improve the test, or make it fail more reliably on master with no satisfying outcome. Tweaking the STALL_TIMEOUT constant, for example, either makes the test less sensitive to regression or more likely to fail on macos. I think we are at the limit of what we can test from the client side. We could add more DebugLog messages to the server and test server-side behavior a bit more like the other throttle test check_slow_read_throttle() but I chose to leave that alone for now.

http: don't register RecvEvent while a request is in-flight

We use TCP backpressure to throttle incoming data from the client.
We release the throttle when there is an empty receive buffer and
no request in the progress of being parsed, but we should continue
to throttle if there is a "busy" request being handled by a worker
thread.

On some platforms this would allow up to 65kB of additional data
to trickle in after the throttle was supposedly engaged.

0066120eb6

DrahtBot added the label RPC/REST/ZMQ on Sep 16, 2026

DrahtBot commented at 3:34 PM on September 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/36276.

Reviews

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

Type	Reviewers
Concept ACK	hodlinator
Approach ACK	winterrdog
User requested bot ignore	151henry151

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

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_http.py] assert sent <= len(flood) * 10, (...) -> assert_greater_than_or_equal(len(flood) * 10, sent, ...)

DrahtBot added the label CI failed on Sep 16, 2026

in src/httpserver.h:535 in 0066120eb6 outdated

 530 | @@ -531,6 +531,9 @@ class HTTPRemoteClient
 531 |       */
 532 |      const HTTPRequest* GetRequest() const { return m_req.get(); }
 533 |  
 534 | +    //! True while a request from this client is being processed by a worker thread.
 535 | +    bool IsRequestBusy() const { return m_req_busy.load(); }

hodlinator commented at 5:36 PM on September 16, 2026:

nanonit: Wish we had a more descriptive name... something closer to:

    bool WorkerHasRequest() const { return m_req_busy.load(); }

hodlinator commented at 5:43 PM on September 16, 2026: contributor

Concept ACK 0066120eb648a588dc83b1d95247aef0e1135bb4

Will have a closer look soon.

test: use select() to avoid blocking on macos in interface_http.py

The flood loop uses a non-blocking socket to detect when the server
stops reading from the connection. When the server stops reading, the
TCP receive buffer fills up and send() is expected to fail immediately
with "try again" (EAGAIN/BlockingIOError).

On macOS 15, when the buffer fills completely to zero, the kernel can
block inside send() waiting for the remote side to free up space,
instead of returning an error like it should. This causes the test to
hang when run alongside other tests that put the system under load.

Fix: call select() with a short timeout before each send(). select()
asks the kernel whether there is space to write, and always returns
within the timeout. If there is no space, we never call send() at all,
so the kernel never gets the chance to block.

0527c052fe

pinheadmz commented at 7:25 PM on September 16, 2026: member

Added a second commit to handle this CI failure where macos-15 just hangs forever. My agents and I think what is happening is that macos is blocking inside of send() even though the socket is non-blocking. So we'll wrap that in a select() call with a timeout to detect the TCP backpressure.

pinheadmz marked this as a draft on Sep 16, 2026

pinheadmz commented at 10:44 PM on September 16, 2026: member

Converting to draft, i think i have this issue again where 50ms is being added to every RPC call...

davidgumberg commented at 12:20 AM on September 17, 2026: contributor

Sorry for the dump, if you understand the issue in the PR description then this is not helpful so I'll put this inside of a details block:

<details> <summary> I had a bit of trouble following what the issue is here, so I just want to check my understanding: </summary>

https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L1034-L1039

The server has an open socket. a. the server's ReadyToSend is False so the first check fails. (where Ready means HaveStuffQueuedToSend not CouldSendStuffIfAskedTo) b. There is no current request, but the ReceiveBuffer is empty. so the else if branch is taken. c. The socket is marked with the receive event, so in WaitMany() we'll look for a POLLIN event:

https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/util/sock.cpp#L171-L173

https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/util/sock.cpp#L188-L190

The client sends a request like waitforblockheight. a. Now there is a RecvEvent in the events.occurred for the client's sock. b. The client sock is recv_ready in SocketHandlerConnected: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L904 and now Receive() is called: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L916-L918 c. Receive() fills the m_recv_buffer: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L964-L968 d. TryReadRequest() is called a request is constructed: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L1082-L1084 e. TryReadRequest() calls ReadRequest() which will read some bytes from the buffer, clearing m_recv_buffer as it goes. f. Eventually the whole request is read, and turned into a job, the m_req is cleared and the client is marked m_req_busy = true: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L1133-L1134
The next time WaitForSockets() is called, m_req == nullptr (m_req_busy == true) and the receive buffer is empty, so the client is marked as ready for a receive again. a. TryReadRequest will get called again on this client, but the receive buffer won't be drained into a new HTTPRequest since the client is still marked as busy with a request until the waitforblockheight completes: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/src/httpserver.cpp#L1080

This is the bug since it means that the client can send the server more data (up to the server's TCP receive window limit which is usually ~64KiB or so).

I think my understanding up to here is right, but I'm a little bit confused about why the functional test was failing, and how it relates to the zero-window probing, but here's what I gather:

Because the 64KiB or so get drained from the kernels buffer into our application request buffer, this frees up the OS's receive buffer. a. the server's kernel's window goes from 0 bytes available, to all bytes available or however many were drained into the application buffer. b.But the server's kernel never advertises this to the client ?? c. Instead the client only learns this at some random time later when it does a zero-window poll. d. Whenever it does the zero-window probe, it actually learns that it can send another full window of data, but even after it does that, the next time WaitForSockets() is called, the application's receive buffer has contents so we don't mark the socket as ready to receive.

On MacOS, the zero-window probe timer happens to be ~5 seconds, so about ~5 seconds into the test, the zero-window probe is performed by the client and it learns that it can send another window of bytes, resetting the stuck_timer:

https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/test/functional/interface_http.py#L767-L777

which we were waiting for to hit 5 seconds: https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/test/functional/interface_http.py#L756-L758

But the test times out after 10 seconds:

https://github.com/bitcoin/bitcoin/blob/b8215855437288cac95c7fd5d41104c73c0f3495/test/functional/interface_http.py#L759-L761

So in some cases there won't be enough time left for the stall to hit 5 seconds, and that's why the test is failing.

And the fix here is to just check if m_req_busy is true, so we don't have this gap where we mean to stop reading from the socket buffer once we're busy with a request, but we actually accidentally read one more time.

in test/functional/interface_http.py:802 in 0527c052fe

 810 |                  # receive buffer while a request is in flight.
 811 |                  raise AssertionError(
 812 |                      f"Server kept reading pipelined data ({sent} bytes) while a "
 813 |                      f"request was still in flight for {PROGRESS_TIMEOUT}s.")
 814 | -            time.sleep(0.05)
 815 |

winterrdog commented at 9:27 PM on September 17, 2026:

since we no longer pipeline requests or read-ahead requests, i think it is reasonable for the test to use a blocking socket to match the new server behaviour (serial processing) and let TCP backpressure the client while the request is in flight

i think we could use a short socket timeout together with the existing select(). in that, select() avoids entering send() when we already know the socket is not writable, while the socket timeout gives us a backstop (defensive) if send() still blocks after select() says it is writable. we can treat socket.timeout the same as BlockingIOError and let STALL_TIMEOUT remain the actual threshold for deciding that the connection is stuck

this also seems to avoid making the 50ms timeout itself a test failure condition. A busy CI machine could legitimately take longer than that to make progress, so we'd rather count it as no progress and let the existing 5s stall threshold (STALL_TIMEOUT) decide

we'd probably also use SEND_TIMEOUT for the select() timeout so there's only one value to keep in sync

<details> <summary>suggested diff </summary>

diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
index 333a4efb70..3a9214d40b 100755
--- a/test/functional/interface_http.py
+++ b/test/functional/interface_http.py
@@ -741,12 +741,13 @@ class HTTPBasicsTest (BitcoinTestFramework):
             f'Content-Length: {len(body)}\r\n\r\n' +
             body
         ).encode("ascii")

-        # Non-blocking send: When the server stops reading from the buffer
-        # due to TCP backpressure, Python will raise an error. If the socket
-        # was set to blocking, we would have to wait for an ambiguous timeout.
-        conn.conn.sock.setblocking(False)
+        # Use a blocking socket with a brief timeout (50ms) as a backstop for
+        # platforms where send() may block even after select() reports the
+        # socket writable
+        SEND_TIMEOUT = 0.05
+        conn.conn.sock.settimeout(SEND_TIMEOUT)

         # Kernel socket buffer sizes vary widely across platforms,
         # so we can't rely on counting sent() bytes to determine if the
         # server is actually draining its end of the socket.
@@ -769,16 +770,21 @@ class HTTPBasicsTest (BitcoinTestFramework):
             # when the TCP receive window reaches zero the kernel can block inside
             # send() doing zero-window probing even on a non-blocking socket,
             # preventing BlockingIOError from ever being raised. select() always
             # honors its timeout regardless of TCP window state.
-            _, writable, _ = select.select([], [conn.conn.sock], [], 0.05)
+            _, writable, _ = select.select([], [conn.conn.sock], [], SEND_TIMEOUT)
             if not writable:
                 # Socket not writable within timeout: kernel send buffer is full.
                 if stuck_since is None:
                     stuck_since = time.monotonic()
                 elif time.monotonic() - stuck_since > STALL_TIMEOUT:
                     # No progress: the server has stopped reading.
                     break
+                # stuck_since is not None by now therefore the
+                # PROGRESS_TIMEOUT check below will not fire on this
+                # path; skip straight to the next select() instead of
+                # checking it again
+                continue
             else:
                 try:
                     sent += conn.conn.sock.send(flood[sent % len(flood):])
                     # Progress: the server is still reading.
@@ -786,10 +792,12 @@ class HTTPBasicsTest (BitcoinTestFramework):
                     self.log.debug(f"sent: {sent}")
                     assert sent <= len(flood) * 10, (
                         f"Server accepted {sent} bytes of pipelined data while a "
                         "request was still in flight: the receive buffer is not throttled")
-                except BlockingIOError:
-                    # Rare race between select() and send(); treat as not writable.
+                except (BlockingIOError, socket.timeout):
+                    # Rare race between select() and send(), or send()
+                    # might still block on some platforms. Treat
+                    # both as no progress
                     if stuck_since is None:
                         stuck_since = time.monotonic()
                     elif time.monotonic() - stuck_since > STALL_TIMEOUT:
                         break

</details>

thoughts ?

winterrdog commented at 9:28 PM on September 17, 2026: contributor

approach ACK

in order to keep request processing strictly serial, as intended in #35735#issue-4901039304, it makes sense to stop pipelining requests or reading requests ahead while one is still in flight

reply-to: #36276 (comment)

i think i have this issue again where 50ms is being added to every RPC call...

i might be missing something here, but where do you think the 50ms would be coming from in the RPC path ? i am trying to understand how it could end up affecting each RPC call

pinheadmz commented at 9:57 PM on September 17, 2026: member

@winterrdog The 50ms is the select timeout which pauses the IO loop when there are no events. Since this patch is reducing events it forces many operations to wait a loop before being executed.

I have a second commit locally that calls TryReadRequest from the worker threads (with a lock) to keep the pipeline moving, but after banging on this a few more hours today (in order to respond properly to @davidgumberg) I'm starting to think the issue really is just a flaky test. The bug I'm chasing here is pretty minimal and I think the approach in this PR so far is going to cost more than it's worth.

I've been running the test through wireshark and watching the actual packets. The zero window probe is happening but the real macOS behavior is much more sinister: as the TCP window gets smaller, the kernel delays transport. It'll even raise a socket blocking error, wait five seconds, then send more data.

151henry151 commented at 4:20 AM on September 18, 2026: contributor

Benchmarked keep-alive RPC latency on Linux (regtest, Python http.client, synchronous client). All busy sockets run in one tick, but per-connection latency has a ~50ms floor. Keep-alive only, a fresh connection per call wakes the loop on accept. Bench: https://gist.github.com/151henry151/ed1bdf7119e4100a97a83880a4a19dda

Compared master 0e9018e8 to tip 0527c052 (merge-base 51ddab532c). Nothing touches httpserver.{h,cpp} or util/sock.{h,cpp} in between and the second commit is test-only, so the delta is 0066120e.

Single connection, n=200 after 20 warmup:

build	`getblockchaininfo` p50 / p99	`getblockcount` p50 / p99
master `0e9018e8`	0.30 / 0.44 ms	0.22 / 0.34 ms
`0527c052`	50.16 / 51.13 ms	50.18 / 51.92 ms

One full SELECT_TIMEOUT per call, not half, so it's every call, not a race. Per-client p50: 1× 50.19; 2× 50.24 / 50.22; 4× 50.11–50.29.

The cost is on the next request, not the response. Send() already optimistic-sends before clearing m_req_busy, but the wait set was built while the socket was busy, so it sits in WaitMany with no events requested. Listen sockets only wake on accept, and ReadyToSend() is usually false after the optimistic send. TryReadRequest from the worker doesn't help here: m_recv_buffer is empty until Receive() runs, and it's unsynchronized, effectively I/O-thread-owned. WaitMany is level-triggered, can't poll without draining.

I think what this needs is a wakeup so clearing m_req_busy re-arms the wait set. util::SignalInterrupt already uses a POSIX TokenPipe self-pipe for that pattern; Windows would need a loopback socketpair, since a pipe fd isn't selectable by the select() fallback in Sock::WaitMany. That would probably be its own PR.

For the test: @winterrdog's socket timeout stops the hang, but if the kernel later lets send() through every ~5s, stuck_since still resets. The oracle needs something other than client-side writability. @davidgumberg re 4b: the stack can advertise a reopened window with an update ACK, not only a probe. @pinheadmz's captures show the client delaying as the window shrinks. Sender-side SWS avoidance (RFC 1122 §4.2.3.4) fits. ~5s progress vs 5s STALL_TIMEOUT means PROGRESS_TIMEOUT is what fires.

pinheadmz commented at 4:18 PM on September 20, 2026: member

moving to https://github.com/bitcoin/bitcoin/pull/36303

pinheadmz closed this on Sep 20, 2026