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

Cable: Gracefully handle disconnected clients #24259

Merged
merged 1 commit into from Mar 23, 2016

Conversation

Projects
None yet
3 participants
@jeremy
Member

jeremy commented Mar 21, 2016

We'll get Errno::ECONNRESET if the client forcibly disconnected.
Just close the socket rather than raising the exception.

Handle other errors in ClientSocket#write, too, mirroring Faye's
error handling which swallows all StandardError on write.

/cc @matthewd

@jeremy

View changes

actioncable/lib/action_cable/connection/stream.rb Outdated
@socket_object.client_gone
raise

This comment has been minimized.

@jeremy

jeremy Mar 21, 2016

Member

Bubble up to ClientSocket where it's rescued and emitted rather than swallowing it entirely.

This comment has been minimized.

@matthewd

matthewd Mar 21, 2016

Member

It won't actually get emitted, because of the ready_state... right? 😕

But, are these "error"-worthy? We heard about it through an exception, but client disappearance is a pretty ordinary occurrence: if we've already handled it, maybe that's all that needs to happen.

This comment has been minimized.

@jeremy

jeremy Mar 21, 2016

Member

Agreed

@@ -36,6 +36,7 @@ def connect
@faye.on(:open) { |event| @event_target.on_open }
@faye.on(:message) { |event| @event_target.on_message(event.data) }
@faye.on(:close) { |event| @event_target.on_close(event.reason, event.code) }
@faye.on(:error) { |event| @event_target.on_error(event.message) }

This comment has been minimized.

@jeremy

jeremy Mar 21, 2016

Member

No test case for this yet.

assert connection.connected
end
end
end

This comment has been minimized.

@jeremy

jeremy Mar 21, 2016

Member

Lots of duplication here. Figure we can eat it for now and incrementally get closer to 1. using the actual classes rather than test stubs and 2. disentangling the classes so they're easier to manipulate and use on their own.

Gracefully handle disconnected clients
We'll get `Errno::ECONNRESET` if the client forcibly disconnected.
Just close the socket rather than raising the exception.

Handle other errors in `ClientSocket#write`, too, mirroring the Faye
error handling which swallows all `StandardError` on write.

@jeremy jeremy force-pushed the jeremy:cable/disconnected-client-errors branch to 4f8a8e2 Mar 21, 2016

@jeremy jeremy merged commit 09e30d9 into rails:master Mar 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremy jeremy deleted the jeremy:cable/disconnected-client-errors branch Mar 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment