-
Notifications
You must be signed in to change notification settings - Fork 97
fix: 'Close received after close' error #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Check closed state before checking opcode to prevent sending multiple Close frames - Return Ok(()) when attempting to send Close on already-closed connection - Call stream.shutdown() after sending Close frame per RFC 6455 - Add tests for double-close behavior Fixes denoland/deno#21642
Per https://datatracker.ietf.org/doc/html/rfc6455#section-7.1.1, TCP Close happens after both endpoints have exchanged Close frames. Calling shutdown immediately after sending a Close frame is premature - the peer hasn't responded yet. The library's write_frame() method should not make this decision; TCP close is left to the application (or occurs when the WebSocket struct is dropped after the handshake completes).
…moved from registry)
|
I see Also, it seems crossbario/autobahn-testsuite:0.8.2 doesn't exist anymore so I set it to latest and now CI passes. |
|
I’m following this issue and wanted to add support for merging this PR. It addresses a real-world problem where multiple Close frames can be emitted, which leads Chromium clients to report 1006 (abnormal closure). The fix is very contained: after a Close is sent, additional Close frames become a no-op, while non-Close frames still error as before. No API or protocol changes beyond write_frame. Tests cover the behavior, existing tests pass, and CI is green. This looks like a solid, low-risk fix worth landing. |
|
|
littledivy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you. I'll test this downstream in Deno and merge.
* fix: prevent double-close and shutdown TCP write-half after Close frame - Check closed state before checking opcode to prevent sending multiple Close frames - Return Ok(()) when attempting to send Close on already-closed connection - Call stream.shutdown() after sending Close frame per RFC 6455 - Add tests for double-close behavior
|
Backported this fix and released 0.8.1 |
When a server initiates a WebSocket close(), the current implementation allows sending multiple Close frames. This manifests as clients receiving code=1006 (abnormal closure) close.
(a 'Close received after close' error in Chromium)
In
WriteHalf::write_frame(), the conditional logic was:The else if only triggers for non-Close frames, so a second Close frame would still be sent.
The Fix:
Check closed state before checking opcode:
I also included the call tostream.shutdown().await?(as in #72) after sending Close frame to properly close the TCP write-half (per RFC 6455).Note:
In PR #72, it was mentioned that the tests were having trouble. They are passing for me with this change and I added additional tests for this bug.
Admittedly I used Claude to create the tests- I'm not a rust master (just being honest!)
... I hope this helps. I really need this to make it upstream- I have a lot personally riding on it (thousands of deployments in airports around the world).
Thanks & Happy Holidays.
Fixes denoland/deno#21642
Closes #72