fix: spool queued request bodies to break HTTP/2 upload deadlock#2541
Draft
dunglas wants to merge 1 commit into
Draft
fix: spool queued request bodies to break HTTP/2 upload deadlock#2541dunglas wants to merge 1 commit into
dunglas wants to merge 1 commit into
Conversation
Concurrent uploads multiplexed on one HTTP/2 connection hang when there are more streams than PHP threads. A stream queued waiting for a thread keeps its flow-control window open while no one reads the body; enough queued streams exhaust the connection-level window and stall every stream on the connection, including those a thread is already serving. Drain a request body into a buffer (spilling past 2 MiB to a temp file) before it enters the queue, releasing the window so every stream is read by someone. Requests that get a thread immediately still stream live. Bodies overrunning a request_body max_size limit are rejected with 413 instead of reaching PHP truncated. Closes #1074
Contributor
|
Would it also be possible to just read a fixed amount of bytes ahead and prevent the deadlock like that? (maybe even based on MaxConcurrentStreams) Reading the whole body seems like it might open up other potential issues regarding DOS and requires managing a separate max body size. Also will lead to files being buffered to disk essentially twice. |
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.
Problem
Concurrent file uploads multiplexed on a single HTTP/2 connection hang indefinitely when there are more streams than PHP threads (#1074).
Root cause is an HTTP/2 flow-control deadlock, not thread-pool exhaustion:
ServeHTTPuntil a PHP thread frees. A queued stream never reads its request body.Body.Readforever. Circular wait.This matches the reproduction: hang at default thread counts, never with separate connections (no multiplexing). With
max_wait_timeunset (infinite), it hangs forever with no error.Fix
Drain a request body into a buffer right before it enters the thread queue, releasing the flow-control window so every stream is read by someone (a thread or the spooler).
spoolRequestBody: buffers to memory, spills past 2 MiB to a temp file, honorsrequest_body_timeoutduring the drain, cleans up the temp file when the request ends.Content-Lengthare spooled; chunked / unknown-length streaming requests keep their live stream.request_body max_sizelimit (http.MaxBytesReader) is rejected with413instead of reaching PHP truncated.Tests
TestConcurrentUploadsHTTP2: 30 concurrent uploads on one h2c connection with 2 threads. Deadlocks without the fix (verified: hangs to the timeout), passes with it.Notes / trade-offs
os.TempDir(), bounded by the client'sContent-Length. Pair withrequest_body max_sizeto cap it.Closes #1074