Repository navigation
fix: end a streamed response after a backpressured write - #377
Open
mustafa519 wants to merge 1 commit into
Open
mustafa519 wants to merge 1 commit into
mustafa519 wants to merge 1 commit into
Conversation
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
A streamed response can hang forever: the whole body is sent, but the response never ends (no terminating chunk), so the client keeps waiting until a proxy or client timeout cuts it.
It happens when:
res.write()(for example piping React'srenderToReadableStream/ anyReadableStreamintores), thenres.end()is called;_res.write()returnsfalse) on the last chunk, sowritingChunkstaystrueuntilonWritablefires;write()returnedtrueandneedDrainis not set.end()then does:Node only emits
'drain'after awrite()that returnedfalse. In case 3 it never does, so the deferredend()never runs, even thoughonWritablelater completes the pending chunk.In practice this needs a slow reader at the very end of the body, for example a reverse proxy with response buffering off that pauses reading once its buffer is full. Direct clients and buffering proxies drain fast enough that uWS rarely reports backpressure on the last chunk, which is probably why it's intermittent.
Fix
end()stores the deferred call in a private#endAfterWriteinstead of waiting for'drain'.#continueEnd()runs it once no chunk is being written. It's called after the write callback in bothonWritablepaths (chunked and known content-length). Running it after the callback lets a callback that starts the next chunk keep the end waiting for that chunk too.Test
tests/tests/res/res-end-after-backpressured-write.jsmakes uWS report backpressure on the last 3-byte chunk, then become writable 50 ms later, whileend()is called with that chunk pending. Before the fix the response never ends (response never endedafter 3 s). After it, the output matches Express.After
npm ci:node tests/index.js res57/57 andnode tests/index.js send-file21/21 pass.Since when
The
'drain'wait inend()is older, but it became reachable for chunked responses in 2.1.0 (45576f6, "remove timer based backpressure handling"). Before that, the chunked_write()ignored uWS backpressure and called the callback right away, sowritingChunkwas nevertruewhenend()ran.