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

Add method ActionCable::Channel#stop_stream_from, ActionCable::Channe… #37171

Merged
merged 5 commits into from Jan 17, 2020

Conversation

@piecehealth
Copy link
Contributor

piecehealth commented Sep 11, 2019

Add methods ActionCable::Channel#stop_stream_from, ActionCable::Channel#stop_stream_for to allow user unsubscribe specify steam.

Summary

Users can follow multiple streams in one channel by calling stream_from or stream_for multiple times. As far as I know, ActionCable only provide ActionCable::Channel#stop_all_streams to unfollow all streams, So I add two methods to allow user unsubscribe specify steam.

…l#stop_stream_for to allow user unsubscibe specify steam.
@rails-bot rails-bot bot added the actioncable label Sep 11, 2019
@@ -1,3 +1,7 @@
* Add method `ActionCable::Channel#stop_stream_from`, `ActionCable::Channel#stop_stream_for` to allow user unsubscibe specify steam.

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

Add ActionCable::Channel#stop_stream_from and #stop_stream_for to unsubscribe from a specific stream.

@@ -82,7 +82,7 @@ def stream_from(broadcasting, callback = nil, coder: nil, &block)
# Build a stream handler by wrapping the user-provided callback with
# a decoder or defaulting to a JSON-decoding retransmitter.
handler = worker_pool_stream_handler(broadcasting, callback || block, coder: coder)
streams << [ broadcasting, handler ]
streams[broadcasting] = [ broadcasting, handler ]

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

Note this behavior change. If we stream_from the same broadcasting with a different handler, we'll overwrite the first handler.

Did we mean to support multiple subscriptions to the same pubsub broadcasting?

This comment has been minimized.

Copy link
@piecehealth

piecehealth Sep 12, 2019

Author Contributor

Good point!
I didn't see any test cases to cover this scenario: user can subscribe to the same pubsub broadcasting with multiple subscriptions, and I think it make sense to overwrite the previous subscription with a new one, just like we can define methods with the same name multiple times, but only last one will work.

What's your opinion? 😉

@@ -82,7 +82,7 @@ def stream_from(broadcasting, callback = nil, coder: nil, &block)
# Build a stream handler by wrapping the user-provided callback with
# a decoder or defaulting to a JSON-decoding retransmitter.
handler = worker_pool_stream_handler(broadcasting, callback || block, coder: coder)
streams << [ broadcasting, handler ]
streams[broadcasting] = [ broadcasting, handler ]

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

(We can also do streams[broadcasting] = handler rather than storing the broadcasting twice.)

@@ -114,7 +128,7 @@ def stop_all_streams
delegate :pubsub, to: :connection

def streams
@_streams ||= []
@_streams_hash ||= {}

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

Can leave this instance variable name unchanged. It wasn't @_streams_array either 😊

channel.stop_all_streams

subscribers = subscribers_of(connection)
assert_equal 1, subscribers_of(connection).size

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

subscribers.size

.pubsub
.subscriber_map
.instance_variable_get(:@subscribers)
end

This comment has been minimized.

Copy link
@jeremy

jeremy Sep 11, 2019

Member

Testing against internal implementation details is brittle, but feels appropriate in this case. Alternatively, we'd need to broadcast and check which subscribers received it.

This comment has been minimized.

Copy link
@piecehealth

piecehealth Sep 12, 2019

Author Contributor

Agree, maybe it's possible to switch test pubsub adapter from actioncable/test/stubs/test_adapter.rb to actioncable/lib/action_cable/subscription_adapter/inline.rb

piecehealth added 3 commits Sep 12, 2019
…tream_from_subscription_confirmation` with Ruby 2.5 randomly.
@piecehealth

This comment has been minimized.

Copy link
Contributor Author

piecehealth commented Sep 12, 2019

Hi @jeremy ,
I did some changes based on your comments, would you mind to take another look 😉 ?

@piecehealth piecehealth requested a review from jeremy Sep 12, 2019
@rails-bot

This comment has been minimized.

Copy link

rails-bot bot commented Dec 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 17, 2019
@rails-bot rails-bot bot closed this Dec 24, 2019
@jeremy jeremy reopened this Jan 17, 2020
@rails-bot rails-bot bot removed the stale label Jan 17, 2020
@jeremy
jeremy approved these changes Jan 17, 2020
@jeremy jeremy merged commit 96b74fe into rails:master Jan 17, 2020
1 of 2 checks passed
1 of 2 checks passed
build
Details
buildkite/rails Build #66521 started
Details
@jeremy jeremy added this to the 6.1.0 milestone Jan 17, 2020
@piecehealth piecehealth deleted the piecehealth:stop_stream branch Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.