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

Add autoprunning of empty rooms #2

Closed
wants to merge 1 commit into from

Conversation

bluehallu
Copy link

No description provided.

@kdbanman
Copy link

The patch is only 6 lines, and the feature seems reasonable and simple. Why hasn't it been merged?

@panuhorsmalahti
Copy link

By the way, it seems that this.sids is not cleared properly when a socket.io connection leaves a room.

Adapter.prototype.del = function(id, room, fn)
    this.sids[id] = this.sids[id] || {};

@gpestana
Copy link

@kdbanman Maybe because the rooms should be persistent even when they are empty. Maybe the best way to implement this functionality is to add a flag to the room indicating either if it should be deleted when empty or not. Or for the sake of simplicity, leave that decision to the upper layers.

@yads
Copy link

yads commented Sep 5, 2014

@gpestana this is likely causing memory leak issues seen here

@nebkam
Copy link

nebkam commented Sep 7, 2014

@gpestana Room creation is implicit - if you try to join a room that doesn't exist, it's created for you. That said, I think it's counter intuitive for the room deletion to be explicit.
Besides that, documentation concerning rooms is a bit misleading:

Upon disconnection, sockets leave all the channels they were part of automatically, and no specially teardown is needed on your part

@rauchg
Copy link
Contributor

rauchg commented Sep 11, 2014

We should have this. Please rebase it.

@rase-
Copy link
Contributor

rase- commented Sep 11, 2014

Thanks @hallucynogenyc! Rebased and merged your fix in #10. :)

@rase- rase- closed this Sep 11, 2014
@bluehallu
Copy link
Author

No problem, was actually rebasing now you saved me the effort 👍

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.

None yet

8 participants