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

Always reconnect after connection closes #4924

Closed

Conversation

laurglia
Copy link
Contributor

Phoenix creates a new WebSocket connection if the current one closes for
some reason. However, for a new connection to be created, it is required
that the following conditions hold:

  • The connection did not exit cleanly. This is needed to not make the
    socket reconnect after it is manually disconnected with
    Socket#disconnect.
  • The WebSocket exit code was not normal (something other than 1000).

In some cases, the connection can close with the 1000 code and still
require reconnecting. For example, when turning the internet off for
around 1 minute, and turning it back on, then the connection sometimes
closes with code 1000.

You can test that behavior on my test site: http://34.219.19.64:5000/

Here is also a video of the issue reproduced by me: https://www.youtube.com/watch?v=j76SvA3kglQ

This commit removes the requirement for the condition that the
connection has to close with code 1000 to make reconnections more
reliable. I found that the condition was first added in November 2021
with no explanation on why it was needed in the first place:
470337d

If there is a good reason why this condition is set in place, do let me
know, but otherwise I would go ahead and remove it.

The new behavior can be tested on my second test site: http://34.219.19.64:5001/

Vide of the new behavior: https://www.youtube.com/watch?v=q06kPuTln9Q

Phoenix creates a new WebSocket connection if the current one closes for
some reason. However, for a new connection to be created, it is required
that the following conditions hold:
- The connection did not exit cleanly. This is needed to not make the
socket reconnect after it is manually disconnected with
`Socket#disconnect`.
- The WebSocket exit code was not normal (something other than 1000).

In some cases, the connection can close with the 1000 code and still
require reconnecting. For example, when turning the internet off for
around 1 minute, and turning it back on, then the connection sometimes
closes with code 1000.

You can test that behavior on my test site: http://34.219.19.64:5000/

Here is also a video of the issue reproduced by me: https://www.youtube.com/watch?v=j76SvA3kglQ

This commit removes the requirement for the condition that the
connection has to close with code 1000 to make reconnections more
reliable. I found that the condition was first added in November 2021
with no explanation on why it was needed in the first place:
phoenixframework@470337d

If there is a good reason why this condition is set in place, do let me
know, but otherwise I would go ahead and remove it.

The new behavior can be tested on my second test site: http://34.219.19.64:5001/

Vide of the new behavior: https://www.youtube.com/watch?v=q06kPuTln9Q
@laurglia
Copy link
Contributor Author

laurglia commented Aug 24, 2022

I may have found the reason why this behavior was needed: #2832

So basically when the server disconnects the user with Endpoint.broadcast(socket.assigns.id, "disconnect", %{}), then they should not reconnect.

I checked that this commit would break that behavior and start reconnecting.

To retain the behavior that we don't reconnect when server closes the connection, I have two ideas:

  • Use a custom close code (4000 for example) on the server side to indicate that the connection was closed by the Phoenix server and that client should not reconnect. That change would be backwards incompatible with Phoenix clients. Here are rough code changes required for the JS client and Phoenix server: salemove@854ce43
  • Send some payload as part of the message that says to the client whether they should or should not reconnect. So if we close the connection with code 1000, then set phx_no_reconnect as close reason to indicate that the client should not reconnect. Here are rough code changes required for JS client and Phoenix server: 0c1664c

@jeregrine
Copy link
Member

Hey I think I might have handled this in a slightly different way here in #5024 while trying to solve a different bug.

@jeregrine jeregrine closed this Oct 14, 2022
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