Repository navigation
fix(http): preserve pipelined response order and receive pauses - #6220
binaryfire wants to merge 2 commits into
Conversation
binaryfire
changed the title
|
Follow-up: this now also keeps |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6220 +/- ##
==========================================
- Coverage 84.08% 83.94% -0.14%
==========================================
Files 115 115
Lines 20237 20285 +48
Branches 3637 3643 +6
==========================================
+ Hits 17016 17028 +12
- Misses 3221 3257 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d31f76b to
a2055bd
Compare
matyhtf
left a comment
matyhtf
left a comment
There was a problem hiding this comment.
The per-connection sequencing is coherent across base and process modes. Intermediate header/chunk writes carry SW_SERVER_SEND_MORE_DATA, while the terminal send (including sendfile) releases the next buffered request only after the output has been accepted into the reactor's ordered send path. resume_http_request() revalidates the session in a deferred callback and respects application pause and queued-input backpressure state.
The explicit recv_paused state also closes both existing implicit-resume paths: output-buffer drain and the queued-input timer. The added tests exercise response ordering in both server modes and both pause regressions.
The red unit result is the unrelated http2_duplex_stall busy-loop regression that was subsequently fixed, and the thread jobs fail during setup because setup-php rejects the 8.x-zts version string; neither failure reaches this patch's code.
2325e6b to
8205534
Compare
|
@matyhtf I’ve retargeted this to |
|
However, I would like to raise some mild reservations about the part that "fully supports preserving the order of HTTP/1.1 pipelined requests," for discussion: The beneficiaries are relatively narrow. HTTP/1.1 pipelining is largely deprecated in the mainstream ecosystem. Browsers either do not support it or have it disabled by default, mainly due to the protocol's inherent head-of-line blocking. In practice, pipelined requests are mostly sent by load-testing tools, certain client libraries, or internal gateways, and such clients can usually be changed to send requests serially. Therefore, the actual benefit of this fix may be somewhat limited in scope. The concurrency advantage of coroutines is hard to realize under pipelining. Because the protocol requires responses to be returned in request order, even if the second request has already entered a coroutine, its response must still wait for the first one to complete before it can be output. The upper bound of this fix is more of a correctness safety net—"no loss, no out-of-order"—rather than an improvement in concurrency. The right direction for multiplexing is probably still HTTP/2 or HTTP/3. |
|
Hi @NathanFreeman. I agree that HTTP/1.1 pipelining is uncommon and that HTTP/2/3 are the better choice for multiplexing. This is intended as a correctness fix, not a concurrency improvement. RFC 9112 still permits pipelining and requires servers to return responses in request order. Today, if the first handler yields and the second finishes first, the client receives the second route’s response as the response to its first request. This Hypervel regression test reproduces that with a real application server. The PR deliberately serializes dynamic request dispatch on each connection instead of buffering complete responses. Unrelated connections remain concurrent, and the fix uses bounded state. A server can’t always control its clients; if a gateway pipelines requests from different users over one upstream connection, reordering can send one user’s response to another. Otherwise, this leaves a real, unhandled edge case for frameworks and applications: they can return the wrong response for valid HTTP/1 traffic, even if the clients that trigger it are relatively uncommon. |
b7c067c to
c613502
Compare
binaryfire
changed the title
…ight Each dynamic HTTP/1 request runs in its own coroutine, and the parser dispatched every complete request already in the receive buffer. When a handler yielded, a later pipelined request could write its response first. A pipelined Expect: 100-continue request could also get its interim response ahead of an earlier response. A connection now dispatches one dynamic request at a time. Later request bytes stay in the buffer, and once some are held, reading stops as one more receive policy beside pause and backpressure. When the response's final output is accepted, the held requests are parsed from a deferred callback without waiting for another read. 100 Continue goes through the connection's send path, behind earlier output. The same hold applies while receiving is off for any other reason. In BASE mode, a handler that paused its connection with another request already buffered left that request stranded after resume(), or dispatched it while paused when output was still pending.
The server releases the next pipelined request when a response's final output is accepted. Headers, chunks and the trailer prefix now carry SW_SERVER_SEND_MORE_DATA in ext_flags, through Server::send() and send-yield retries, so only the final write releases it. A raw Server::send() after detach() counts as the final output. Tests cover response order across header-only and body requests and sendfile, and pipelined requests on a paused connection, in BASE and PROCESS modes.
c613502 to
08b6e2e
Compare
|
@matyhtf I've rebased this onto #6384 and reworked it to fit the new receive policies:
Could you take another look when you have a moment? |
A pipelined HTTP/1 connection can dispatch a later request while an earlier handler is suspended. Both handlers write directly to the same connection, so the later response can arrive first.
This dispatches one dynamic HTTP/1 request at a time on each connection. Later request bytes stay in the receive buffer, and once some are held, reading stops through the receive policies from #6384, beside pause and backpressure. When the final response output is accepted, the held requests are parsed from a deferred callback without waiting for another read. Completion uses the existing send metadata without buffering or copying whole responses.
The same gate applies when a request follows a fixed-length or chunked body, preserving the body-boundary parsing and validation added by #6205. All three
100 Continuepaths use the normal connection output path, so an interim response can't overtake earlier buffered output.Buffered requests are also held while receiving is paused. In BASE mode, a handler that called
Server::pause()with another request already buffered left that request stranded afterServer::resume(), or dispatched it while paused when the earlier response was still being sent. In PROCESS mode the buffered request was dispatched while paused.The regressions fail on unchanged master in BASE and PROCESS modes. They cover a yielding header-only request followed by a yielding body request and another pipelined request, a sendfile response with delete-after-send, and pipelined requests on a paused connection whose earlier response has either finished or is still being sent.
Hypervel request handlers routinely yield on coroutine I/O, so this is reachable in normal framework use. This Hypervel regression currently fails on affected Swoole versions.