Fix HTTP/1 idle streamable response disconnect#225
Draft
samuel-williams-shopify wants to merge 1 commit into
Draft
Fix HTTP/1 idle streamable response disconnect#225samuel-williams-shopify wants to merge 1 commit into
samuel-williams-shopify wants to merge 1 commit into
Conversation
HTTP/1 streamable response bodies were previously consumed through the generic body.read path for normal chunked responses. For Protocol::HTTP::Body::Streamable this hides the producing body block behind an internal scheduled fiber. If the body writes an initial chunk and then parks idle, the HTTP/1 server waits on the next body.read, the user body fiber waits independently, and a peer disconnect does not necessarily trigger another socket read or write. The body fiber can therefore remain parked indefinitely. Handle normal HTTP/1.1 streamable responses with a protocol-owned body task instead. The task calls body.call(stream) directly and writes chunks through a small ChunkedOutput adapter, preserving chunked transfer framing while giving the HTTP/1 connection ownership of the task lifecycle. A transient monitor watches for peer close using the existing non-consuming readable? probe and closes the connection only when the socket is no longer readable, avoiding false positives for request-body data or pipelined bytes. Non-streamable bodies, HEAD responses, HTTP/1.0, upgrades, and tunnels continue using the existing paths so ordinary responses do not pay the extra task cost. Add a regression test for issue #224 that opens an HTTP/1.1 streaming response, reads the first SSE chunk, closes the client socket while the body is idle, and asserts the body block unwinds. Closes #224
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
1 participant
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.
TL;DR
Fix HTTP/1.1 idle streamable response bodies so their body task is stopped when the client disconnects.
Context
Closes #224.
HTTP/1 normal responses consumed streamable bodies through
body.read. ForProtocol::HTTP::Body::Streamable, that schedules the body block in an internal fiber. If the body writes one chunk and then parks idle, the server waits for the nextbody.readand the producer fiber waits independently. A client disconnect with no pending application read/write can leave the producer fiber parked indefinitely.Changes
ChunkedOutputadapter instead of the genericbody.readpath.Server#closed, mirroring the HTTP/2 lifecycle more closely.readable?probe so request-body or pipelined data does not get treated as disconnect.Tophatting
bundle updatebundle exec sus test/protocol/http/body/streamable.rbbundle exec sus test/async/http/protocol/http11.rbbundle exec sus test/async/http/protocol/http11.rb test/protocol/http/body/streamable.rbbundle exec bake test