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

Fix `unsubscribed` server side behavior #23715

Merged
merged 1 commit into from Feb 19, 2016

Conversation

Projects
None yet
3 participants
@maclover7
Member

maclover7 commented Feb 16, 2016

Closes #23708

Before this commit, the unsubscribed callbacks in Action Cable server
side channels were never called. This is because when a WebSocket
"goodbye" message was sent from the client, the Action Cable server
didn't properly clean up after the now closed WebSocket. This means that
memory could possibly skyrocket with this behavior, since part of this
commit is to properly remove closed subscriptions from the global
subscriptions hash. Say you have 10,000 users currently connected, and
then all 10,000 disconnect -- before this patch, Action Cable would
still hold onto information (and Ruby objects!) for all of these now
dead connections.

cc @dhh @matthewd

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 17, 2016

Good catch. Test coverage to demonstrate the expected behavior?

@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 17, 2016

Was working on this earlier, but added a regression test to show the expected behavior. cc @matthewd since we talked about this in Campfire.

@maclover7 maclover7 force-pushed the maclover7:fix-unsubscribe branch Feb 17, 2016

@maclover7

View changes

actioncable/test/client_test.rb Outdated
c.send_message command: 'subscribe', identifier: JSON.dump(channel: 'EchoChannel')
assert_equal({"identifier"=>"{\"channel\":\"EchoChannel\"}", "type"=>"confirm_subscription"}, c.read_message)
assert_equal(1, app.connections.count)
assert_equal(1, app.connections.first.subscriptions.method(:subscriptions).call.count)

This comment has been minimized.

@maclover7

maclover7 Feb 17, 2016

Member

This is ugly (and definitely not public API), but is still necessary to demonstrate the regression test.

@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 18, 2016

@jeremy updated with regression test - all set to go?

@matthewd

View changes

actioncable/lib/action_cable/connection/subscriptions.rb Outdated
@@ -54,7 +54,7 @@ def identifiers
end
def unsubscribe_from_all
subscriptions.each { |id, channel| channel.unsubscribe_from_channel }
subscriptions.each { |id, channel| remove({ 'identifier' => id }) }

This comment has been minimized.

@matthewd

matthewd Feb 18, 2016

Member

Can we use remove_subscription(channel) here? This feels like unnecessary indirection (and I think I'd actually argue we don't want the log lines).

@matthewd

View changes

actioncable/test/client_test.rb Outdated
assert_equal(1, app.connections.count)
assert_equal(1, app.connections.first.subscriptions.method(:subscriptions).call.count)
channel = app.connections.first.subscriptions.method(:subscriptions).call.first[1]

This comment has been minimized.

@matthewd

matthewd Feb 18, 2016

Member

method(:subscriptions).call? 😕

This comment has been minimized.

@maclover7

maclover7 Feb 18, 2016

Member

I hate having this line, but I think this is the only way to get access to the EchoChannel instance -- as far as I can see, there is no non-weird way about that (or public API).

@matthewd

View changes

actioncable/test/client_test.rb Outdated
assert_equal(1, app.connections.first.subscriptions.method(:subscriptions).call.count)
channel = app.connections.first.subscriptions.method(:subscriptions).call.first[1]
channel.expects(:unsubscribed).returns('Goodbye from EchoChannel!')

This comment has been minimized.

@matthewd

matthewd Feb 18, 2016

Member

Is this returns doing what you think it is / intend it to?

This comment has been minimized.

@maclover7

maclover7 Feb 18, 2016

Member

Yes -- I want to make sure that the unsubscribed method is properly called and handled.

@maclover7 maclover7 force-pushed the maclover7:fix-unsubscribe branch 3 times, most recently Feb 18, 2016

Fix `unsubscribed` server side behavior
Before this commit, the `unsubscribed` callbacks in Action Cable server
side channels were never called. This is because when a WebSocket
"goodbye" message was sent from the client, the Action Cable server
didn't properly clean up after the now closed WebSocket. This means that
memory could possibly skyrocket with this behavior, since part of this
commit is to properly remove closed subscriptions from the global
subscriptions hash. Say you have 10,000 users currently connected, and
then all 10,000 disconnect -- before this patch, Action Cable would
still hold onto information (and Ruby objects!) for all of these now
dead connections.

@maclover7 maclover7 force-pushed the maclover7:fix-unsubscribe branch to cefcc0f Feb 18, 2016

matthewd added a commit that referenced this pull request Feb 19, 2016

Merge pull request #23715 from maclover7/fix-unsubscribe
Fix `unsubscribed` server side behavior

@matthewd matthewd merged commit 6da571a into rails:master Feb 19, 2016

1 check passed

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

@maclover7 maclover7 deleted the maclover7:fix-unsubscribe branch Feb 19, 2016

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