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

websocket disconnect/leave-all-rooms should delete empty rooms as well otherwise rooms hash goes ever growing #862

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

akiraho
Copy link
Contributor

@akiraho akiraho commented Jan 5, 2018

websocket/server.leave() already has the code to remove a connection from the room, and in case it's the last connection in the room then also remove the room from the server rooms hash table.

however websocket/server.LeaveAll() duplicated the code to remove the connection from the room but without removing the room itself from the hash table if it's then empty. note that with the current architecture the rooms hash isn't used just for rooms but it's for every single websocket connection as well. so this makes the rooms hash ever growing with every connection and re-connection. (a hash key pointing to an empty array value)

this commit is to simply re-use server.leave() from server.LeaveAll(), it already fixes the ever-growing-hash issue.

@akiraho akiraho requested a review from kataras as a code owner January 5, 2018 02:02
@CLAassistant
Copy link

CLAassistant commented Jan 5, 2018

CLA assistant check
All committers have signed the CLA.

@kataras
Copy link
Owner

kataras commented Jan 5, 2018

Hey @akiraho , thank you very much for your interest to contribute here!

I had my reasons for that duplication of the leave code there, performance minor reasons (on LeaveAll we have the rooms and looping trough them, in the s.leave it looks for the room name again from the map) but I assume that is fine, you tested it on many clients too (I assume) , so it's consired to be OK cost.

A small tip: at your code, the _ is not necessary: when you use only the key of a map in the range loop, you don't have to accept the value, so we could omit the _ and have it like for name := range s.rooms... .

Thank you very much, approved!

@kataras kataras merged commit 07d5c74 into kataras:master Jan 5, 2018
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.

3 participants