fix(websocket): implement receive backpressure for WebSocketStream#5539
Open
GiHoon1123 wants to merge 1 commit into
Open
fix(websocket): implement receive backpressure for WebSocketStream#5539GiHoon1123 wants to merge 1 commit into
GiHoon1123 wants to merge 1 commit into
Conversation
The readable stream never had a pull algorithm and every message was enqueued unconditionally, so a slow consumer let the underlying socket buffer an unbounded number of messages in memory instead of pausing. Track both the parser's own buffer pressure and the readable stream's desiredSize, and only resume the socket once neither is backpressured. Also resume unconditionally when closing/cancelling, since the closing handshake needs to keep reading off the socket to see the peer's Close frame - otherwise a socket paused for receive backpressure would leave `closed` unresolved forever. Fixes: nodejs#5503
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.
This relates to...
Fixes #5503
Rationale
WebSocketStream's readable side never applied backpressure. TheReadableStreamwas constructed without apullalgorithm, so consumer demand (desiredSize) was never observable, and both spec steps that say "Apply backpressure to the WebSocket" (constructor step 7, and the message-receive algorithm step 4) wereTODO/no-op. Every parsed message was enqueued unconditionally regardless of whether the consumer was reading, so a slow (or absent) reader let the socket buffer an unbounded number of messages in memory — the issue's repro shows RSS growing from 75 MiB to 337 MiB with 200k unread 1 KiB messages, and the same amount fully flushed on the wire even though the client had read almost nothing.The existing
onSocketData→parser.write() === false→socket.pause()/onParserDrain→socket.resume()pair only reflects the parser's own internal buffer draining as fast as frames can be parsed — it has nothing to do with consumer demand.Changes
Two backpressure sources now gate the socket independently:
onSocketDatapauses whenparser.write()returnsfalse;onParserDrainclears that flag.pullalgorithm on theReadableStreamre-checks on everyreader.read();#onMessagepauses the socket oncedesiredSize <= 0after enqueueing.The socket only resumes once both are clear (parser not congested and
desiredSize > 0) — either source alone is enough to keep it paused.While testing this I found a second, related bug: once
close()/reader.cancel()starts the closing handshake, it needs to keep reading off the socket to see the peer's own Close frame. If the socket happened to be paused for receive backpressure at that moment (a very reachable state — any unread backlog leaves it paused), the peer's Close frame was never read andclosedhung forever. Fixed by unconditionally resuming the socket once closing starts (#resumeSocketForClose) — there's no more reader to protect from backpressure at that point, so ignoring both backpressure sources above is intentional, not an oversight.Features
N/A
Bug Fixes
WebSocketStreamreader no longer buffers unbounded messages in memory or lets the sender flush unlimited data past what the consumer has actually read — receive backpressure now propagates down to pausing the socket, and from there to the underlying TCP connection.close()/reader.cancel()no longer wait forever on the peer's Close frame if the socket happened to be paused for receive backpressure when closing started.Breaking Changes and Deprecations
N/A —
WebSocketStreamis still experimental (UNDICI-WSSwarning). Consumers that read promptly see no behavior change; slow/absent consumers now correctly experience backpressure instead of unbounded buffering.Status