Skip to content
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

Handle websocket close events differently #5024

Closed
wants to merge 4 commits into from
Closed

Conversation

jeregrine
Copy link
Member

@jeregrine jeregrine commented Oct 14, 2022

Right now today when we "disconnect" a websocket we simply use stop reply and let cowboy handle the rest, where as cowboy has a reply shutdown that allows you to send the status code and a message.

I chose 3000 as it is currently registered with the iana as "unauthorized" the entire 3000-3999 status code space is left open for libraries and frameworks to register. We could claim 3001 for "kicked" or something for example.

This solves #4924 as we can simply check for error code 3000 in the ws onConnClose case.

The second issue is that when a user navigates away from the page SOME browsers will initate a websocket close event with status 1001 (going away) and isTrusted set to true. This was causing the connection status flash in liveview to flicker if you navigated away using a standard a tag href.

Work left to do:

  • Test in Firefox
  • Test in chrome
  • Test in Safari
  • Test in mobile Safari
  • Test in various android browsers?

@jeregrine jeregrine marked this pull request as ready for review October 14, 2022 20:54
@jeregrine jeregrine changed the title Handle Channel close events differently Handle websocket close events differently Oct 14, 2022
@chrismccord
Copy link
Member

@jeregrine we can revisit this when you have some time now that the bandit adapter work has settled. Note that the flash issue was solved in LV so we don't necessarily need that part of the fix here.

@mtrudel
Copy link
Member

mtrudel commented Jan 20, 2023

I've got some additive changes to websock that I'm hoping to find time for in the next few weeks (see phoenixframework/websock_adapter#2) which would help somewhat with this (at the very least, it should be coordinated)

@mtrudel
Copy link
Member

mtrudel commented Feb 21, 2023

I just pushed some new proposed functionality up to address phoenixframework/websock_adapter#2. May be of interest here as well.

@jeregrine
Copy link
Member Author

@chrismccord I agree we don't need most of this code ATM but I do think we can use close codes to handle the "user is kicked" scenario more cleanly.

@jeregrine jeregrine closed this May 4, 2023
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.

None yet

3 participants