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

When using useChannel channel never detaches even when all listeners have unsubscribed #1594

Open
thenano opened this issue Jan 25, 2024 · 6 comments
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@thenano
Copy link

thenano commented Jan 25, 2024

It looks like the detach on this line never runs, because channel.listeners.length always evaluates as 1.

Please forgive me if my investigation is way off and I have looked in the wrong place. From what I could tell, listeners in the Channel is actually a function, defined by EventEmitter, which would return any listeners when called without and event, or the event specific listeners when called with an event.
Apart from that, it looks like the channel's subscriptions are actually held in the channel's subscriptions attribute, which is also an instance of EventEmitter, so this would be the right place to look for remaining subscriptions?
It looks to me that to ensure there are no more listeners in the channel and we can safely detach, one would have to check all any/events/anyOnce/eventsOnce subscriptions? Something like:

if (channel.subscriptions.any.length === 0 && channel.subscriptions.anyOnce.length === 0 && Object.keys(channel.subscriptions.events).length === 0 && Object.keys(channel.subscriptions.eventsOnce).length === 0) {
  await channel.detach();
}

or some sort of !some loop on all subscriptions?

Happy to do a pull request is any of this makes sense? Apologies if I'm doing something wrong.

┆Issue is synchronized with this Jira Bug by Unito

Copy link

sync-by-unito bot commented Jan 25, 2024

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-4054

@VeskeR
Copy link
Contributor

VeskeR commented Jan 29, 2024

Hello @thenano !

Thank you for bringing this up. The issue here is that, as you've correctly noticed, channel.listeners is a method that comes from the EventEmitter class, but it's never actually called on this line. Instead it accesses length property on listeners, which just returns the number of parameters expected by the function (which is always 1 listeners).

This is a bug and will be fixed. I'll let you know when we've released a fix.

@VeskeR VeskeR added the bug Something isn't working. It's clear that this does need to be fixed. label Jan 29, 2024
@thenano
Copy link
Author

thenano commented Jan 30, 2024

Thanks very much for the reply @VeskeR
I just also want to point out that, as far as I can tell, even if useChannel is changed to call the listeners method instead of just accessing length, I don't think it will correctly check for all listeners, since the method either returns any listeners when no parameter is passed, or just the listeners for the specified event when that is passed?
As far as I understand, we'd want to check for listeners for all events plus any listeners? And perhaps even for eventsOnce and anyOnce listeners?

@zameschua
Copy link

zameschua commented May 9, 2024

Hey @VeskeR , I think I ran into the same issue.

I was using the imperative API previously and was manually detaching the channels.
When switched from the imperative API to react hooks recently, the following error appears in production:

Maximum number of channels per connection (200) exceeded

I traced it back to the same line of code.
For my app, we use ChannelProvider and useChannel() for multiple different channels, depending on the screen being rendered.
So subscribes and unsubscribes happen quite often when screens mount and unmount.

Let me know if there's anyway I can help debug the issue more or patch it on my end first.

Version that I'm on:
"ably": "^2.0.3"

@zameschua
Copy link

Hey @VeskeR or @sacOO7,

Is there any way this bug can be prioritized? It's currently breaking in production. (Sorry for being annoying)

@mechastriker3
Copy link

Bumping this issue again as we are facing the same issues as @zameschua

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

No branches or pull requests

4 participants