Send Bulk leave event instead of multiple leave events on disconnect #928
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
mrniko
requested changes
Aug 23, 2023
src/main/java/com/corundumstudio/socketio/namespace/Namespace.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were on a rather old version of this library, so ran into #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 #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
The obvious problem here that I do see is we send pubsub event before actually leaving the rooms on this node.
Has a memory overhead for copying the joinedRooms set
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.