fix(http): don't reset shared req/resp while async response is in flight (#424) - #867
Merged
Merged
Conversation
ithewei
force-pushed
the
fix/http-pipeline-async-crash
branch
from
August 6, 2026 07:16
36918b2 to
f431569
Compare
…ght (#424) HttpHandler::Reset() reuses the same HttpRequest/HttpResponse objects. On a keep-alive connection, if a new request arrives while an async handler on a worker thread is still producing/sending the previous response, FeedRecvData called Reset() unconditionally -- racing the worker's use of resp/writer and risking a crash (reported in #424; also reachable via HTTP pipelining or a malicious peer). Guard it: when a new request arrives and the handler isn't back at WANT_RECV, only Reset() if the response has already been handed off (writer->isEnd(), i.e. End() was called -- the terminal call in every writer usage sequence). If the async response is still in flight, reject the early/pipelined data (ERR_REQUEST) so the connection is closed instead of corrupting in-use objects. The completion signal is the writer's end state, not HttpHandler::state: an async writer writes straight to the socket and never advances the handler's send-state machine, so HttpHandler::state stays HANDLE_CONTINUE. Also add HttpResponseWriter::isBegin()/isEnd() predicates (matching isHttp2()) and use them instead of poking writer->state/end directly. Verified via raw-socket single-connection keep-alive tests: sync 5/5, async 5/5, async-with-WriteResponse 5/5, interleaved 8/8; non-keepalive (Connection: close) still closes the connection after the response; pipeline-during-async is rejected without crashing; make check ~650k keep-alive reqs OK. Alternative to #814 that keeps req/resp reuse.
ithewei
force-pushed
the
fix/http-pipeline-async-crash
branch
from
August 6, 2026 07:19
f431569 to
5de7d74
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #424 — potential crash when a new request arrives on a keep-alive connection while an async handler is still producing/sending the previous response (also reachable via HTTP pipelining or a malicious peer).
Root cause
HttpHandler::Reset()reuses the sameHttpRequest/HttpResponseobjects. InFeedRecvData, when the handler wasn't back atWANT_RECV, it calledReset()unconditionally. If an async handler on a worker thread still holdsresp/writerand is writing, that reset races the worker → data race / crash.Fix
Only
Reset()when the previous response has actually been handed off. The completion signal is the writer'send == SEND_END, notHttpHandler::state: an async writer (HttpResponseWriter, itself aSocketChannel) writes straight to the socket and never advances the handler's send-state machine, soHttpHandler::statestaysHANDLE_CONTINUEfor the whole async response.Verified
curl --next, single connection)Connection #0, no spurious close (an earlierstate-based attempt broke this; the writer-endsignal is the correct one)/slowasync 500ms + immediately pipelined/fast): connection rejected/closed, server does not crash and keeps servingmake check: ~658k keep-alive requests, all OK (no throughput/regression)Relation to #814
#814 fixes the same crash by allocating fresh
req/resp/writerinReset(). This PR is a smaller alternative that keeps the existing object-reuse and just refuses to reset while the async response is in flight (per the "reject a new request before the previous response is sent" approach). Either addresses #424; this one is a more contained change.