Skip to content

fix: complete Duplex finalization after socket termination - #2338

Open
tianrking wants to merge 1 commit into
websockets:masterfrom
tianrking:codex/fix-stream-final-after-close
Open

tianrking wants to merge 1 commit into
websockets:masterfrom
tianrking:codex/fix-stream-final-after-close

Conversation

@tianrking

Copy link
Copy Markdown

Calling WebSocket.terminate() destroys the underlying socket without necessarily finishing its writable side. If the wrapped Duplex is then ended, _final() waits for a socket finish event that cannot occur, leaving its end callback and writable finish pending.

Complete the finalizer when the socket is already destroyed. Also check both readable end and writable finish from either event, so the Duplex closes after both halves complete regardless of event order and preserves unread buffered data. The existing failed-handshake guard and finalizer callback ordering are retained.

Six real TCP client/server regressions cover CLOSED and CLOSING states, readable end before writable finish, buffered data, and allowHalfOpen: false, including exactly-once lifecycle events and end callbacks.

Validation on exact commit 51510804a47dfc010983bc802706f5bb0d2dd599:

  • Original native RED: on Node 10 and 24, two terminated client/server cases fail actual completion assertions while two graceful controls pass; production source is unchanged.
  • Native matrix and lint: Node 10/22/24 Ubuntu and Node 24 Windows/macOS pass the regressions, original probe, complete npm test, and both ws/wss echo integrations. Full Node 10/24 optional-addon fallback suites and unchanged ESLint/Prettier checks pass. The aggregate run is red because its direct Mocha CLI invocation fails before tests on Node 26.
  • Node 26 verification: unchanged baseline npm test passes 438 cases; exact formal npm test passes 444, the six regressions and original four probes pass, and lib/stream.js has 100% coverage. Both trees have the existing single HTTP-header test skip. Bare npm run integration fails before tests in both trees because legacy yargs/yargs is loaded as an ES module; executing the same two real integration files through the project's unchanged nyc/Mocha loader passes on both. No dependency, package-script, or CI change is included.

Source hashes and clean working trees are checked before and after native validation.

@lpinca

lpinca commented Oct 5, 2026

Copy link
Copy Markdown
Member

You should not use the wrapped WebSocket directly. Use duplex.destroy() instead of websocket.terminate().

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants