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

Allow subscribing to the same channel once from a connection #36

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

mjeffrey18
Copy link
Contributor

Purpose

To keep close to the ActionCable way of doing things, this change gets us a step closer.

The problem

Rails doesn't allow the same connection to subscribe to the same channel many times.
Cable.cr does at the moment, unfortunately.
It actually causes unnecessary memory bloat by allowing the same client connection to get connected many times, receiving many of the same broadcasted messages also

Solution

Silently do nothing if the client makes this mistake. This is exactly what rails does...
Works perfectly.

Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
…nnection

Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize this was connecting multiple times. I don't think I've seen that in my app, but I have seen where sometimes it just doesn't connect... This doesn't affect anything in a case where someone connects, then refreshes the browser, right?

spec/cable/connection_spec.cr Show resolved Hide resolved
@mjeffrey18
Copy link
Contributor Author

I didn't realize this was connecting multiple times. I don't think I've seen that in my app, but I have seen where sometimes it just doesn't connect... This doesn't affect anything in a case where someone connects, then refreshes the browser, right?

It will work fine with refreshes and across browser windows and tabs, we have 1 app with some legacy code that creates 2 connections and subscribed to different channels - all good. Plus we have opened dozens of tabs, windows etc and they all get a new connection and can subscribe without issue with these changes. We just need to protect the server from accepting many subscriptions on the same channel via the same connection as it won't do the server any good and also the client will just get duplicate messages so not useful to anyone. Better to stick with what rails does and just ignore the client if it makes those mistakes.

Copy link
Collaborator

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Looks good 👍

Copy link
Contributor

@russ russ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this happened in our app a while back. We put quite a bit of logic in place on the client to try and make it wasn't doing it. I like having the confidence server side.

@@ -24,16 +24,13 @@ describe Cable::Connection do

it "accepts without params hash key" do
connect do |connection, socket|
connection.receive({"command" => "subscribe", "identifier" => {channel: "ChatChannel", room: "1"}.to_json}.to_json)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took me a while to figure out about this change, then I read the test description 🤦‍♂️ lol

@fernandes fernandes merged commit 3701f50 into cable-cr:master Nov 18, 2021
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.

None yet

4 participants