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

Added support for bulk join and leave #873

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

ksahu750
Copy link

We have a large number of rooms a socket client can subscribe to, and when a client connects to our server, we pull all the rooms they were previously subscribed to from our backend and issue a join request for all the rooms, this also ends up issuing a publish on pubsub (async wrapper over RedissonPubSubStore in our case) for JOIN / LEAVE events
This is causing high CPU spikes

Added a bulk JOIN / LEAVE API to allow the above use case to be handled in a more efficient manner for us. This will allow client to join a large number of rooms at the same time without causing a lot of resource strain

We have a large number of rooms a socket client can subscribe to, and when a client connects to our server, we pull all the rooms they were previously subscribed to from our backend and issue a join request for all the rooms, this also ends up issuing a publish on pubsub (redis in our case) for JOIN / LEAVE events
This is causing high CPU spikes

Added a bulk JOIN / LEAVE API to allow the above use case to be handled in a more efficient manner for us. This will allow client to join a large number of rooms at the same time without causing a lot of resource strain

Bump version

Fix - Changed join to leave in leaveRooms

Fix - Using Sets instead of List, as it makes more sense semantically here

Cleaned up imports/formatting

Update serialVersionUID
@mrniko mrniko merged commit d4c726d into mrniko:master Nov 22, 2022
@mrniko
Copy link
Owner

mrniko commented Nov 22, 2022

Thanks for contribution!

ksahu750 pushed a commit to ksahu750/netty-socketio that referenced this pull request Aug 2, 2023
We were on a rather old version of this library, so ran into
mrniko#668, saw the issue was
open so tried a fix, which worked for us

Checked the later version and found that this was fixed in commit
452870d, but the issue with the current approach is it issues leave
event one by one for all the joined rooms, which puts a lot of stress
on pubsub store if there are a lot of joined rooms.

Due to the above issue we had high CPU spikes, and had added this
feature mrniko#873, a little
while back.

Thinking of leveraging that here to send leave event for all the joined
rooms at once, to reduce the stress on pubsub store.

Few possible solutions, that I can think of
1. Do a bulk leave beforehand, as joinedRooms set gets modified on
   leaving room
   The obvious problem here that I do see is we send pubsub event
   before actually leaving the rooms on this node.
2. Copy joinedRooms and do bulk leave after leaving
   Has a memory overhead for copying the joinedRooms set
3. Have a separate event altogether like LEAVE_ALL (essentially same as
   DISCONNECT)
4. Currently, the default DISCONNECT subscription we have in
   BaseStoreFactory is just a debug log, we can handle it there itself,
   but only sessionId is propagated there, namespace is not, we can
   either iterate all namespaces, or pass joined namespaces of the
   client in that packet.

Would like to know your opinion, or if there are better suggestions.
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.

2 participants