fix(sse): register client before attaching close handler to fix race (#1128)#1405
Open
jakebromberg wants to merge 1 commit into
Open
fix(sse): register client before attaching close handler to fix race (#1128)#1405jakebromberg wants to merge 1 commit into
jakebromberg wants to merge 1 commit into
Conversation
…1128) If the underlying socket closed between `client.res.on('close', ...)` and the final `this.clients.set(client.id, client)`, the close handler's `unsubAll(client.id)` ran against a map that didn't contain the client — silent no-op. The client was then inserted into `this.clients` and lived there forever, leaking memory and producing write-to-dead-socket attempts on subsequent broadcasts. Move the `clients.set` ahead of the close handler attach and ahead of writeHead/write so the synchronous `'close'` window can't observe a not-yet-inserted entry. No async boundary sits between the set and the on('close') attach. Tests cover the close-during-write window via a mock Response whose `write` synchronously emits 'close'.
3 tasks
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.
Closes #1128.
Problem
ServerEventsManager.registerClientattachedclient.res.on('close', ...)before the finalthis.clients.set(client.id, client). If the underlying socket closed in that window (e.g. a TCP RST while the SSE headers were still being flushed), the close handler'sunsubAll(client.id)ran against a map that didn't yet contain the client — a silent no-op. The subsequentclients.setthen inserted the (already-dead) client, where it lived forever: no inactivity timer, no live socket, and a guaranteed write-to-dead-socket attempt the next time anything broadcast to one of its topics.Fix
Move the
clients.set(client.id, client)ahead ofstartHeartbeat, ahead of theon('close')attach, and ahead of thewriteHead/initial write — eliminating the window in which'close'can fire against a not-yet-registered entry. No async boundary sits between the set and the on('close') attach, so the close handler is guaranteed to observe the inserted client when it runs.Test
tests/unit/utils/serverEvents.test.tsadds aregisterClient close-before-insert race (BS#1128)block with two cases. Both use a mockResponsewhosewritesynchronously emits'close', mimicking a socket reset arriving mid-writeHead/write:subscribe(...)for that client throws a 404-styleWxycError(the client was correctly removed from the map by the close handler).disconnect(...)for that client does NOT callres.end()(no dead entry leaked into the map fordisconnectto act on).Both tests fail against
mainand pass after the reorder.Local CI
npm run lint— 0 errorsnpm run format:check— cleannpm run typecheck— cleannpm run test:unit— 3153 passed (incl. 17 inserverEvents.test.ts, up from 15)