Repository navigation
Don't treat code 1000 as special - #6880
Merged
Merged
Conversation
Originally introduced in #2832, the close code 1000 was treated as a way to make the documented "disconnect" broadcast not reconnect in the browser. See also #3161 (comment) The problem is that code 1000 is not always in our control. A middleware might close a WebSocket connection with code 1000, leaving the browser permanently disconnected. So using code 1000 is not a realiable meachnism for this. Additionally, we recently changed "disconnect" to send 1001 instead, circumventing the whole thing: #6678 With all that in mind, I propose to remove special casing of 1000 completely. I see two ways we can go forward: 1. Use a custom code, e.g. in 4000-4999 for "disconnect" and document that the socket will not reconnect. LiveView can still handle this code specially, but instead of reloading with jitter, we can immediately reload, as it is most commonly used when disconnecting a single user. 2. Keep "disconnect" reconnecting, as it does now and introduce a new "permanent_disconnect" or however it would be called that is documented to not reconnect. Relates to: #2832 Relates to: #3327 Relates to: 470337d Relates to: phoenixframework/phoenix_live_view@302833c Relates to: phoenixframework/phoenix_live_view@953a862 Relates to: phoenixframework/phoenix_live_view#2421 Relates to: mtrudel/bandit#582 Relates to: #6678
Member
|
We should revert this now, as it hardcodes a status code on the socket that only makes sense for websocket: #6678 |
SteffenDE
marked this pull request as ready for review
October 20, 2026 09:33
Member
Author
|
So I think we keep things as is and revisit if someone requests a "permanent disconnect" kind of thing. If a channel is joined, people can always push a disconnect on that channel and then call |
This reverts commit b6a4e31. Now that phoenix.js reconnects after any close by the server, the close code is no longer needed to make the client reconnect after "disconnect". Bandit now closes normally and waits for the client, so channels exit with {:shutdown, :peer_closed} after a "disconnect" broadcast.
SteffenDE
added a commit
that referenced
this pull request
Oct 20, 2026
* Don't treat code 1000 as special Originally introduced in #2832, the close code 1000 was treated as a way to make the documented "disconnect" broadcast not reconnect in the browser. See also #3161 (comment) The problem is that code 1000 is not always in our control. A middleware might close a WebSocket connection with code 1000, leaving the browser permanently disconnected. So using code 1000 is not a realiable meachnism for this. Additionally, we recently changed "disconnect" to send 1001 instead, circumventing the whole thing: #6678 With all that in mind, I propose to remove special casing of 1000 completely. I see two ways we can go forward: 1. Use a custom code, e.g. in 4000-4999 for "disconnect" and document that the socket will not reconnect. LiveView can still handle this code specially, but instead of reloading with jitter, we can immediately reload, as it is most commonly used when disconnecting a single user. 2. Keep "disconnect" reconnecting, as it does now and introduce a new "permanent_disconnect" or however it would be called that is documented to not reconnect. Relates to: #2832 Relates to: #3327 Relates to: 470337d Relates to: phoenixframework/phoenix_live_view@302833c Relates to: phoenixframework/phoenix_live_view@953a862 Relates to: phoenixframework/phoenix_live_view#2421 Relates to: mtrudel/bandit#582 Relates to: #6678 * update tests and comments * Revert "Make websocket disconnect codes explicit (#6678)" This reverts commit b6a4e31. Now that phoenix.js reconnects after any close by the server, the close code is no longer needed to make the client reconnect after "disconnect". Bandit now closes normally and waits for the client, so channels exit with {:shutdown, :peer_closed} after a "disconnect" broadcast. * Accept local_closed when Bandit handles a disconnect broadcast
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.
Originally introduced in #2832, the close code 1000 was treated as a way to make the documented "disconnect" broadcast not reconnect in the browser.
See also #3161 (comment)
The problem is that code 1000 is not always in our control. A middleware might close a WebSocket connection with code 1000, leaving the browser permanently disconnected. So using code 1000 is not a realiable meachnism for this.
Additionally, we recently changed "disconnect" to send 1001 instead, circumventing the whole thing: #6678
With all that in mind, I propose to remove special casing of 1000 completely. I see two ways we can go forward:
Relates to: #2832
Relates to: #3327
Relates to: 470337d
Relates to: phoenixframework/phoenix_live_view@302833c
Relates to: phoenixframework/phoenix_live_view@953a862
Relates to: phoenixframework/phoenix_live_view#2421
Relates to: mtrudel/bandit#582
Relates to: #6678