Repository navigation
Conversation
If a subscription for the same identifier already exists then it has already been or is being subscribed via the connection, so no need to re-subscribe. Otherwise the guarantor can get confused and attempt to subscribe continually.
1a8c8a4 to
c559967
Compare
If a subscription is created with an identifier already in subscriptions, and that subscription is not pending in the subscription guarantor (meaning it is already connected) then fire the connected callback immediately.
dce50ac to
a205aca
Compare
|
I've added another commit which also immediately fires the connected callback for already-subscribed connections. Otherwise such subscriptions might miss initial state setup, etc. |
Check if the subscription exists before adding the new subscription, or it always will exist.
|
(The tests on CI seem broken for unrelated reasons?) |
There was a problem hiding this comment.
@sj26 Thanks for working on this, and great report and troubleshooting in #44652.
I have added a couple of comments, but this change makes a lot of sense 👍. Could you please:
- Add an entry to the
actionableCHANGELOG. - Add tests for the changes
subscriptions_test.jsandsubscription_guarantor_test.js. I know those aren't super comprehensive right now, but good to at least test the new behavior.
Thanks! I'll make sure we get this merged.
| this.subscribe(subscription) | ||
| if (!existing) { | ||
| this.subscribe(subscription) | ||
| } else if (!this.guarantor.pendingSubscriptions.some((s) => s.identifier === subscription.identifier)) { |
There was a problem hiding this comment.
I'd add a method to SubscriptionGuarantor to avoid having to access pendingSubscriptions from the outside. E.g: !this.guarantor.hasPendingSubcription(subscription.identifier)
| this.consumer.ensureActiveConnection() | ||
| this.notify(subscription, "initialized") | ||
| this.subscribe(subscription) | ||
| if (!existing) { |
There was a problem hiding this comment.
I'd extract this.findAll(subscription.identifier).length > 0 to a method (e.g: contains(identitfier)) and also remove the separated local variable existing. You could do:
if (!contains(identifier)) {
...
}
If I understand it correctly, we create two subscription objects, right? The problem may occur if we decide to unsubscribe: the server would unsubscribe the client, while another subscription objet might not know about that and could try to use this stale subscription. |
|
There would be multiple client side subscriptions, and one server subscription. When the last client subscription is unsubscribed, the server subscription will also be unsubscribed and removed. If another client subscription is then created, it will create a new server subscription. |
|
If I were architecting the javascript, I would have an M:N layer multiplexing the client and server subscriptions which also manages the subscription process, instead of the subscription guarantor. |
|
Anyone know if |
|
Sorry I don't have any updates. I do think this is still an issue. We carry this patch for it in our codebase. But I don't have capacity to push it to completion myself. If someone else is keen, please let me know 🙏 |
|
This is affecting us too. It would be fantastic if the PR got merged. |
palkan
left a comment
palkan
left a comment
There was a problem hiding this comment.
It would be fantastic if the PR got merged.
It wouldn't be. This PR is far from solving the problem and may introduce new ones. In addition to some comments I left in the code, there are more things missing:
-
We do not handle the
subscriptions.reload()scenario (very important, because reconnections happen often) -
No tests verifying that this hack works (or not)—that's the biggest problem with this PR.
I think, a proper fix must be implemented within the SubscirpitonGuarantor (which is the root cause of the problem); we already have there a check if the subscription has been monitored, but it compares subscription objects not identifiers:
Using identifiers instead could probably do the trick. Still. we need a decent test coverage for such race conditions (and that's gonna be the hardest part of the potential PR).
Ideally, we must also take into account unsubscribe requests, especially those happening concurrently with double subscribes. And that would require a proper locking mechanism (not this ad-hoc guarantor thing). This is something we implemented in @anycable/web a while ago—feel free to use it as a drop-in replacement for @rails/actioncable (no need to switch servers for that).
| if (!existing) { | ||
| this.subscribe(subscription) | ||
| } else if (!this.guarantor.pendingSubscriptions.some((s) => s.identifier === subscription.identifier)) { | ||
| this.notify(subscription, "connected") |
There was a problem hiding this comment.
Why connected? If it's pending, it could also be rejected as well (or left without any response from the server); we shouldn't mark it as connected here.
…le JS client Closes rails#44653
…le JS client Closes rails#44653
…le JS client Closes rails#44653
…le JS client Closes rails#44653
This branch has not been deployed
5 participants
If a subscription for the same identifier already exists then it has already been or is being subscribed via the connection, so no need to re-subscribe. Without this change, a subscription message is sent to the server and the guarantor waits for a subscription confirmation which never comes, and so keeps sending subscription messages over and over every second. Fixes #44652.
I'm not sure this is a comprehensive solve, and I'm not sure yet how to test it. I'd love some feedback on the approach. 🙏
I don't think short-circuiting a subscription like this will be a problem. I can't find a place where subscription confirmations are shared back to subscriptions for handling.