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 support for getting clients in a given room #1630

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@FREEZX

FREEZX commented Jun 17, 2014

Added a clients function that calls the adapter's clients function, in order to get the connected clients in a room.
Pull requests implementing the underlying adapter functions:
Redis adapter: socketio/socket.io-redis#15
Memory adapter: socketio/socket.io-adapter#5

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jun 17, 2014

Contributor

Room should be optional right?

Contributor

rauchg commented Jun 17, 2014

Room should be optional right?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jun 17, 2014

Contributor

clients(fn) should work

Contributor

rauchg commented Jun 17, 2014

clients(fn) should work

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jun 17, 2014

Contributor

Thanks for submitting this btw!

Contributor

rauchg commented Jun 17, 2014

Thanks for submitting this btw!

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Jun 17, 2014

Sorry, i only made it for getting clients in a given room, though i'll try to do that tomorrow.
Please also check the mentioned pull requests when you can, the redis one needs a little attention, because i have no idea how to properly clean up the database on exit.

FREEZX commented Jun 17, 2014

Sorry, i only made it for getting clients in a given room, though i'll try to do that tomorrow.
Please also check the mentioned pull requests when you can, the redis one needs a little attention, because i have no idea how to properly clean up the database on exit.

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Jun 19, 2014

I made the base adapter work without the room parameter. I'll add this into the redis adapter as well, though i still need to do something to clean up redis on normal exit.

FREEZX commented Jun 19, 2014

I made the base adapter work without the room parameter. I'll add this into the redis adapter as well, though i still need to do something to clean up redis on normal exit.

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam commented Jun 20, 2014

👍

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment

panuhorsmalahti commented Jun 24, 2014

+1

@mikaturunen

This comment has been minimized.

Show comment
Hide comment
@mikaturunen

mikaturunen commented Jun 24, 2014

👍

@hestinr12

This comment has been minimized.

Show comment
Hide comment
@hestinr12

hestinr12 Jun 24, 2014

Not to blow up this thread, but you already can get the list of clients in a room.

io['sockets']['adapter']['rooms'] returns a dictionary of all the rooms currently open as the key and a dictionary of all clients and their current status in the room as the value (key:value).

Is this not sufficient? Am I missing something? Should this data not be accessed directly or is this PR merely a convenience thing?

hestinr12 commented Jun 24, 2014

Not to blow up this thread, but you already can get the list of clients in a room.

io['sockets']['adapter']['rooms'] returns a dictionary of all the rooms currently open as the key and a dictionary of all clients and their current status in the room as the value (key:value).

Is this not sufficient? Am I missing something? Should this data not be accessed directly or is this PR merely a convenience thing?

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Jun 24, 2014

That doesn't give a list of all the clients connected to all the rooms. It's a convenience I guess, but makes migration from socket-io 0.9 to 1.0 a lot easier.

panuhorsmalahti commented Jun 24, 2014

That doesn't give a list of all the clients connected to all the rooms. It's a convenience I guess, but makes migration from socket-io 0.9 to 1.0 a lot easier.

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Jun 24, 2014

Also, it doesn't work in a multi node environment.

FREEZX commented Jun 24, 2014

Also, it doesn't work in a multi node environment.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 24, 2014

@hestinr12 Now we have an official redis adapter and there's no way to get that dictionary from redis adapter. A convenient API for getting clients is pretty much required for some use cases as discussed earlier in #1428

ghost commented Jun 24, 2014

@hestinr12 Now we have an official redis adapter and there's no way to get that dictionary from redis adapter. A convenient API for getting clients is pretty much required for some use cases as discussed earlier in #1428

@hestinr12

This comment has been minimized.

Show comment
Hide comment
@hestinr12

hestinr12 Jun 24, 2014

@panuhorsmalahti in what case does it not?

@FREEZX mostly out of curiosity, why does that occur? Is this just a reference to the local instance's connections?

@bdemirkir so to clarify, this is provides an interface to the redis cache handling the multi-node environment?

hestinr12 commented Jun 24, 2014

@panuhorsmalahti in what case does it not?

@FREEZX mostly out of curiosity, why does that occur? Is this just a reference to the local instance's connections?

@bdemirkir so to clarify, this is provides an interface to the redis cache handling the multi-node environment?

@ghost

This comment has been minimized.

Show comment
Hide comment

ghost commented Jun 24, 2014

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Jun 24, 2014

@hestinr12
As I understand it, you would need to convert that object of rooms of clients to clients to get the 0.9 functionality (and filter out duplicates). Only a few lines of code, but it makes the migration easier. More importantly, most programmers are not aware of io['sockets']['adapter']['rooms']. It should be documented in the migration guide at least.

panuhorsmalahti commented Jun 24, 2014

@hestinr12
As I understand it, you would need to convert that object of rooms of clients to clients to get the 0.9 functionality (and filter out duplicates). Only a few lines of code, but it makes the migration easier. More importantly, most programmers are not aware of io['sockets']['adapter']['rooms']. It should be documented in the migration guide at least.

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Jun 25, 2014

Hmm,

io['sockets']['adapter']['sids'] ===
{"T6DaL1e3HD8y41KYAAAA":{"T6DaL1e3HD8y41KYAAAA":true},"UzoTueQjgPvQbWusAAAB":{"UzoTueQjgPvQbWusAAAB":true} /* ... */

So I can get an array of socket ids, but how can I get the actual socket connections using the ids?

panuhorsmalahti commented Jun 25, 2014

Hmm,

io['sockets']['adapter']['sids'] ===
{"T6DaL1e3HD8y41KYAAAA":{"T6DaL1e3HD8y41KYAAAA":true},"UzoTueQjgPvQbWusAAAB":{"UzoTueQjgPvQbWusAAAB":true} /* ... */

So I can get an array of socket ids, but how can I get the actual socket connections using the ids?

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Jun 25, 2014

@panuhorsmalahti try with io.sockets.connected[id]

FREEZX commented Jun 25, 2014

@panuhorsmalahti try with io.sockets.connected[id]

@indrekj

This comment has been minimized.

Show comment
Hide comment
@indrekj

indrekj commented Jun 30, 2014

+1

@paulosborne

This comment has been minimized.

Show comment
Hide comment
@paulosborne

paulosborne commented Jul 8, 2014

+1

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Jul 8, 2014

Any updates from maintainers on this status of this PR? :shipit: Looking forward to using 1.0 😄

julianlam commented Jul 8, 2014

Any updates from maintainers on this status of this PR? :shipit: Looking forward to using 1.0 😄

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas commented Jul 25, 2014

+1

@Technowix

This comment has been minimized.

Show comment
Hide comment
@Technowix

Technowix Aug 29, 2014

Merge merge merge merge !

Technowix commented Aug 29, 2014

Merge merge merge merge !

@cnvo

This comment has been minimized.

Show comment
Hide comment
@cnvo

cnvo Aug 29, 2014

Looks good to merge. 👍

cnvo commented Aug 29, 2014

Looks good to merge. 👍

@psychobunny

This comment has been minimized.

Show comment
Hide comment
@psychobunny

psychobunny commented Aug 29, 2014

👍

@a5mith

This comment has been minimized.

Show comment
Hide comment
@a5mith

a5mith Aug 29, 2014

Henry Mergington here. I approve this Pull Request. Do it! Do it naooo.

a5mith commented Aug 29, 2014

Henry Mergington here. I approve this Pull Request. Do it! Do it naooo.

@dbrugne

This comment has been minimized.

Show comment
Hide comment
@dbrugne

dbrugne Aug 29, 2014

+1 to merge, i use it for two weeks now and it seems to work well!

dbrugne commented Aug 29, 2014

+1 to merge, i use it for two weeks now and it seems to work well!

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Aug 29, 2014

The only issue is the redis adapter doesn't clean up

FREEZX commented Aug 29, 2014

The only issue is the redis adapter doesn't clean up

@dbrugne

This comment has been minimized.

Show comment
Hide comment
@dbrugne

dbrugne Aug 29, 2014

@FREEZX, could you describe again what is left with cleanup and normal exit?

dbrugne commented Aug 29, 2014

@FREEZX, could you describe again what is left with cleanup and normal exit?

@FREEZX

This comment has been minimized.

Show comment
Hide comment
@FREEZX

FREEZX Aug 29, 2014

@dbrugne All of node's exit callbacks do not support async events.
Therefore i was not able to do a cleanup (removing clients from rooms, removing rooms from clients).
The file in question is here. The process exits before the async redis calls in the exitHandler function are finished.
https://github.com/FREEZX/socket.io-redis/blob/170aa26d9ec3407a68fd7260eb6984630f934ed7/index.js
As an alternative i was considering cleanup on start, but i haven't tried it yet.

FREEZX commented Aug 29, 2014

@dbrugne All of node's exit callbacks do not support async events.
Therefore i was not able to do a cleanup (removing clients from rooms, removing rooms from clients).
The file in question is here. The process exits before the async redis calls in the exitHandler function are finished.
https://github.com/FREEZX/socket.io-redis/blob/170aa26d9ec3407a68fd7260eb6984630f934ed7/index.js
As an alternative i was considering cleanup on start, but i haven't tried it yet.

dbrugne and others added some commits Sep 15, 2014

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Oct 29, 2014

Hi @guille, congrats on shipping 1.2.0, love the software 😄 -- can we expect this merged for next release?

julianlam commented Oct 29, 2014

Hi @guille, congrats on shipping 1.2.0, love the software 😄 -- can we expect this merged for next release?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Oct 29, 2014

Contributor

This is definitely a major goal for 1.3.0
On Tue, Oct 28, 2014 at 6:19 PM Julian Lam notifications@github.com wrote:

Hi @guille https://github.com/guille, congrats on shipping 1.2.0 [image:
😄] -- can we expect this merged for next release?


Reply to this email directly or view it on GitHub
#1630 (comment).

Contributor

rauchg commented Oct 29, 2014

This is definitely a major goal for 1.3.0
On Tue, Oct 28, 2014 at 6:19 PM Julian Lam notifications@github.com wrote:

Hi @guille https://github.com/guille, congrats on shipping 1.2.0 [image:
😄] -- can we expect this merged for next release?


Reply to this email directly or view it on GitHub
#1630 (comment).

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Nov 21, 2014

Is there anything blocking this merge? It's a must-have feature.

panuhorsmalahti commented Nov 21, 2014

Is there anything blocking this merge? It's a must-have feature.

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 25, 2014

Contributor

It's coming in the next release.

On Fri Nov 21 2014 at 5:28:02 AM Panu Horsmalahti notifications@github.com
wrote:

Is there anything blocking this merge? It's a must-have feature.


Reply to this email directly or view it on GitHub
#1630 (comment).

Contributor

rauchg commented Nov 25, 2014

It's coming in the next release.

On Fri Nov 21 2014 at 5:28:02 AM Panu Horsmalahti notifications@github.com
wrote:

Is there anything blocking this merge? It's a must-have feature.


Reply to this email directly or view it on GitHub
#1630 (comment).

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Jan 10, 2015

Was this actually merged, or just closed?

julianlam commented Jan 10, 2015

Was this actually merged, or just closed?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jan 10, 2015

Contributor

We have another issue tracking this I believe. Is this why you closed @defunctzombie ?

Contributor

rauchg commented Jan 10, 2015

We have another issue tracking this I believe. Is this why you closed @defunctzombie ?

@defunctzombie

This comment has been minimized.

Show comment
Hide comment
@defunctzombie

defunctzombie Jan 10, 2015

Contributor

I thought it was merged per the comment of "coming in next release". Sorry if that is not the case yet.

Contributor

defunctzombie commented Jan 10, 2015

I thought it was merged per the comment of "coming in next release". Sorry if that is not the case yet.

@defunctzombie defunctzombie reopened this Jan 10, 2015

@rase- rase- added the In Progress label Jan 10, 2015

@defunctzombie

This comment has been minimized.

Show comment
Hide comment
@defunctzombie

defunctzombie Jan 10, 2015

Contributor

If this is ready for merge, could the OP please cleanup the commit history.

Contributor

defunctzombie commented Jan 10, 2015

If this is ready for merge, could the OP please cleanup the commit history.

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jan 10, 2015

Contributor

It's not though.

Contributor

rauchg commented Jan 10, 2015

It's not though.

@panuhorsmalahti

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Jan 10, 2015

In which version is this feature coming?

panuhorsmalahti commented Jan 10, 2015

In which version is this feature coming?

@bcharbonnier

This comment has been minimized.

Show comment
Hide comment
@bcharbonnier

bcharbonnier Jan 21, 2015

Is it part of 1.3.x or not ? I can't see it in the release notes changes .....
I mean, the real question is: did the bug get fixed somehow via another PR ?

bcharbonnier commented Jan 21, 2015

Is it part of 1.3.x or not ? I can't see it in the release notes changes .....
I mean, the real question is: did the bug get fixed somehow via another PR ?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jan 21, 2015

Contributor

It's the top priority for our next release. #1945

Contributor

rauchg commented Jan 21, 2015

It's the top priority for our next release. #1945

@bcharbonnier

This comment has been minimized.

Show comment
Hide comment
@bcharbonnier

bcharbonnier Jan 21, 2015

Great! Thanks :)

bcharbonnier commented Jan 21, 2015

Great! Thanks :)

@hayksaakian

This comment has been minimized.

Show comment
Hide comment
@hayksaakian

hayksaakian May 27, 2015

Not clear if this is in or not? (It's been a few months since last comment on this pull)

hayksaakian commented May 27, 2015

Not clear if this is in or not? (It's been a few months since last comment on this pull)

@marksyzm

This comment has been minimized.

Show comment
Hide comment
@marksyzm

marksyzm Jun 2, 2015

:) It is not. I'm guessing... soon.

marksyzm commented Jun 2, 2015

:) It is not. I'm guessing... soon.

@luin

This comment has been minimized.

Show comment
Hide comment
@luin

luin Jun 12, 2015

Any updates on this pr?

luin commented Jun 12, 2015

Any updates on this pr?

@emadd

This comment has been minimized.

Show comment
Hide comment
@emadd

emadd commented Jul 3, 2015

+1

@austinkelleher

This comment has been minimized.

Show comment
Hide comment

austinkelleher commented Jul 19, 2015

+1

@wanming

This comment has been minimized.

Show comment
Hide comment
@wanming

wanming Jul 31, 2015

getClientsInRoom: (io, roomName) ->
    ret = []
    room = io.sockets.adapter.rooms[roomName]

    return ret if !room

    for socketID of room
      ret.push(io.sockets.connected[socketID])
    ret

wanming commented Jul 31, 2015

getClientsInRoom: (io, roomName) ->
    ret = []
    room = io.sockets.adapter.rooms[roomName]

    return ret if !room

    for socketID of room
      ret.push(io.sockets.connected[socketID])
    ret
@benkaiser

This comment has been minimized.

Show comment
Hide comment
@benkaiser

benkaiser commented Oct 26, 2015

+1

@samuel281

This comment has been minimized.

Show comment
Hide comment
@samuel281

samuel281 Jan 21, 2016

I'm still waiting for this feature. Is there any blocker?

samuel281 commented Jan 21, 2016

I'm still waiting for this feature. Is there any blocker?

@LordMajestros

This comment has been minimized.

Show comment
Hide comment
@LordMajestros

LordMajestros Jan 21, 2016

@samuel281 needs to be rebased

LordMajestros commented Jan 21, 2016

@samuel281 needs to be rebased

@Shadowstep33

This comment has been minimized.

Show comment
Hide comment

Shadowstep33 commented Feb 2, 2016

+1

@Shadowstep33

This comment has been minimized.

Show comment
Hide comment
@Shadowstep33

Shadowstep33 Feb 2, 2016

io['sockets']['adapter']['sids'] is returning an empty literal for me even though a client is clearly connected

Shadowstep33 commented Feb 2, 2016

io['sockets']['adapter']['sids'] is returning an empty literal for me even though a client is clearly connected

@JCMais

This comment has been minimized.

Show comment
Hide comment
@JCMais

JCMais May 30, 2016

For anyone interested, I'm currently using the following to accomplish that:

io.in( room ).clients(function( error, clients ) {
    if (error) throw error;
    for ( var i = 0, len = clients.length; i < len; i++ ) {
        io.sockets.connected[clients[i]].emit( 'something', {some: 'data'} );
    }
});

JCMais commented May 30, 2016

For anyone interested, I'm currently using the following to accomplish that:

io.in( room ).clients(function( error, clients ) {
    if (error) throw error;
    for ( var i = 0, len = clients.length; i < len; i++ ) {
        io.sockets.connected[clients[i]].emit( 'something', {some: 'data'} );
    }
});
@darrachequesne

This comment has been minimized.

Show comment
Hide comment
Contributor

darrachequesne commented Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment