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

Improve Action Cable reconnection reliability #23813

Merged
merged 8 commits into from Feb 23, 2016

Conversation

Projects
None yet
6 participants
@lifo
Member

lifo commented Feb 22, 2016

There three improvements here:

  1. Treat "closing" state as "closed" when attempting to reconnect from the client side. We had tons of issues where websocket connections somehow get stuck in the "closing" state, which would prevent automatic reconnection and require a reload.
  2. Add client side logging to make it easier to debug client side connection issues. This is turned off by default, but can be turned on with calling ActionCable.startDebugging() from inside your application's javascript.
  3. Makes sure the client side ActionCable.ConnectionMonitor#connected() gets called upon successful websocket connection.
@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 22, 2016

Can you try to squash down your commits?

@lifo

This comment has been minimized.

Member

lifo commented Feb 22, 2016

What's the benefit of squashing the commits?

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 22, 2016

No need to squash so long as the commits are atomic changes that can be independently reverted.

Would rebase out the faye-websocket switch+revert and the master merge. Keep the git history free of merge loops where we can.

@lifo

This comment has been minimized.

Member

lifo commented Feb 22, 2016

@jeremy Yeah, I thought about removing those faye-websocket switch+revert commits. But it can be useful in case we want to revert that change in the master branch. The previous revert commit was very out of date. But I can swing either ways.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 23, 2016

This looks good to me but I agree with rebasing to remove those three commits.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 23, 2016

javan and others added some commits Feb 17, 2016

Treat 'closing' state as closed.
We are seeing cases where the websockets get stuck in the 'closing' state
after a tab has been in background for a while. So lets treat those websockets
as closed.
Uninstall event handlers when replacing WebSocket instance
Ensures we don't get "onclose" events from a previous WebSocket that was in the "closing" state

dhh added a commit that referenced this pull request Feb 23, 2016

Merge pull request #23813 from lifo/faye-websocket
Improve Action Cable reconnection reliability

@dhh dhh merged commit b2c2d32 into rails:master Feb 23, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment