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 support for getting clients in a given room (Works across multiple nodes) #15

Closed
wants to merge 19 commits into from

Conversation

FREEZX
Copy link

@FREEZX FREEZX commented Jun 17, 2014

I added basic support for the redis adapter for storing and getting clients (As an async call) or a room in the redis database.
I didn't manage to do the exit cleanup correctly, so after normal exit, rooms are still filled up with the same (now unexisting) socket ids. Quitting with Ctrl+C or the kill signal should work as expected.

Also added a test for the clients function call

@olive75
Copy link

olive75 commented Aug 8, 2014

@FREEZX, @guille I corrected some little bugs in FREEZX/pull/1, FREEZX/pull/2 and FREEZX/pull/3

For the sync of clients and rooms data, I am considering the following:

  • Redis stores all keys needed to build local copy of room state (Adapter.sids and Adapter.rooms)
  • socket.io instances build a local copy of a room state from Redis when a client connects to a room that doesn't exists yet in the local copy
  • I am not sure how to sync all rooms local copies there on : in Adapter build in function add, del, delAll or calling Adapter.broadcast via socket.emit directly in the application)
  • when a socket.io node in the cluster crash, ctrl+C,... it either cleanup itself all keys it emitted or publish a leavall message so that the event is handled by another node in the cluster. In both case, it means to setup the Redis key to something like "prefix#clusternodeid#id"

What do you think?

Add client.id argument to the call of Adapter.del
Corrected deletion key on disconnect
@rauchg
Copy link
Contributor

rauchg commented Sep 5, 2014

This is definitely headed in the right direction. Happy to proceed with adding this API. One thing i'd like, however, is to first start by documenting the clients API in socket.io-adapter and socket.io README. Once we merge the in-memory implementation, we can proceed with the Redis one.

@rauchg
Copy link
Contributor

rauchg commented Sep 5, 2014

I think we also need to document the schema we're keeping in Redis on this repository's README. Especially in light of the added data complexity this patch will introduce, and the possibility for improper / incomplete cleanup in the event of a crash.

@dbrugne
Copy link

dbrugne commented Sep 10, 2014

+1 to complete the README to allow users make their own cleanup implementation.

But even I know this merge is waited for a long time I think adding the node uid (as a record) on each socket SET in Redis could be helpful (at least for cleanup implementation).

In future releases adding a node SET on startup (with expire and refresh) will allow a node to determine the state of other nodes easily (and socket from other nodes).

@dbrugne
Copy link

dbrugne commented Sep 15, 2014

@FREEZX, @guille,

I proposed the requesting documentation updates in this pull request: FREEZX#4

@FREEZX
Copy link
Author

FREEZX commented Sep 15, 2014

@dbrugne approved.

@dbrugne
Copy link

dbrugne commented Sep 15, 2014

@FREEZX, same for socket.io clients method: FREEZX/socket.io#1

@FREEZX
Copy link
Author

FREEZX commented Sep 15, 2014

@dbrugne Yep, cool.

@hems
Copy link

hems commented Jul 2, 2015

I didn't manage to check the source code for this pull request, but i'm wondering if a 'pure redis' implementation wouldn't be the case?

As in: each worker saving it's own state on redis?

user enters 
 ~> add user.id to room:id SET
 ~> add room.id user:in_rooms SET

user leaves
 for each id in user:in_rooms SET
   <~ remove user.id to room:id SET

server shutdown
  for each socket.id in server.socket.clients
    for each id in user:in_rooms SET
      <~ remove user.id to room:id SET

The only issue i can see is if the worker doesn't shutdown gracefully, other than that this implementation should solve the issue?

@artofspeed
Copy link

@FREEZX I plan to use this pull request until it's patched in the official API. However, I'm wondering how to actually use your clients() function to get all clients in a room.

Currently I'm doing this:

// set up the adapter
var adapter = require('socket.io-redis')({ host: 'localhost', port: 6379 })
io.adapter(adapter);

// Get all clients in a room
adapter.prototype.clients(room, function(err, data){
    console.log(data); // ['K0ce8P0WP_4vM2evAAAB', ...] This works!
});

This seems to work, but is accessing .prototype a good way to do it?

@FREEZX
Copy link
Author

FREEZX commented Aug 15, 2015

@artofspeed Are you using my fork of socket.io?
You should be able to do io.clients() with my fork.

@hems
Copy link

hems commented Aug 19, 2015

@FREEZX with your fork can we know when a user entered or left a room?

i had to implement this on my application and end up having to do a couple of workarounds with sockets.id which i don't even know if it's unique across multiple instances running.

@ebourmalo
Copy link

@rauchg @FREEZX Any news about the merge of this PR please :)?

@FREEZX
Copy link
Author

FREEZX commented Sep 18, 2015

I do not need this anymore, i've switched to primus with the ws transformer, so i won't be working on this year old PR. If anyone wants to take over, be my guest.

@perrin4869
Copy link

@FREEZX can primus-cluster and primus-rooms find out the number of clients inside a room out of the box?

@ebourmalo
Copy link

@FREEZX interesting. what plugin do you use to replace this redis adapter and broadcasts messages to running node instances with primus ?

@jsdream
Copy link

jsdream commented Sep 25, 2015

I'm also interested in this :)

@FREEZX
Copy link
Author

FREEZX commented Sep 26, 2015

@perrin4869 Only if you use https://github.com/cayasso/primus-rooms#primusroomroomclientsfn and then count the number of results. It should be fairly straightforward to add a count feature.
@ebourmalo You can use https://github.com/cayasso/primus-rooms with https://github.com/mmalecki/primus-redis-rooms

@luislobo
Copy link
Contributor

+1, any news on this?

@jsdream
Copy link

jsdream commented Nov 12, 2015

@rauchg Any chance this going to be merged at all? Lots of people interested in the feature.
Is this not being merged because of clean up limitation on exit or there is something else?

@jsdream
Copy link

jsdream commented Nov 13, 2015

Hello @FREEZX!

Looks like I also decided to go with Primus, but not sure which transformer is better to use. Is ws a viable enough solution?

@jsdream
Copy link

jsdream commented Nov 13, 2015

Just found a nice article on topic of choosing transformers (with performance tests): https://medium.com/@denizozger/finding-the-right-node-js-websocket-implementation-b63bfca0539

@FREEZX
Copy link
Author

FREEZX commented Nov 13, 2015

@jsdream I made a detailed benchmark including all node websocket technologies i found while developing my own web framework in order to choose the best possible approach:
http://nukejs.com/#/benchmarks
https://www.dropbox.com/s/6dgnftmjcjf2g5o/bare_conf.pdf

@jsdream
Copy link

jsdream commented Nov 13, 2015

@FREEZX Thank you very much for the response. Btw, your framework looks very interesting. Will consider it for future projects ;-)

@FREEZX
Copy link
Author

FREEZX commented Nov 13, 2015

@jsdream Thanks, hope you enjoy using it. :) I was previously working with mean.js, but it had some bugs, and it was difficult making it suit my needs, work with websockets and making it scalable, so instead i went out and did this.

@jsdream
Copy link

jsdream commented Nov 13, 2015

@FREEZX Yeah, currently I struggling with MEAN.js to get websockets working in cluster mode. On the benchmark you've provided I noticed this: "Nuke.js uses the websockets transformer by default, because it is the fastest transformer available, and does not require sticky sessions support on your load balancer."

Does this really mean there is no handshake problem when using ws module with primus and node.js cluster? Sorry for such kind of questions, I am just really new to websockets.

@FREEZX
Copy link
Author

FREEZX commented Nov 14, 2015

@jsdream Yes, since it doesn't require sticky sessions, and judging from the manual, that's the only issue that breaks primus with cluster. I also remember asking the same question on the primus IRC, i think i got the same response.
If you have other questions, join them on IRC:

server: irc.freenode.net
room: #primus

@jsdream
Copy link

jsdream commented Nov 14, 2015

Cool. Thank you again!

@stephengardner
Copy link

There's been a lot of discussion going on here. Inability to support this is kind of making me look towards other libraries...

@Jokero
Copy link

Jokero commented Apr 11, 2016

@rauchg Two years... Will it be merged?

@austinkelleher
Copy link

+1 What's the status on this? @rauchg

@JCMais
Copy link

JCMais commented May 30, 2016

This PR is outdated, and broken since new updates to both socket.io and the adapter. As the PR author has abandoned it (which I cannot condemn), it's probably not going to get merged any time soon, if ever.

@felixSchl
Copy link

I have had an idea: how about for the lifetime of the connected socket we periodically set a key on redis with a TTL of some time. If the socket disconnects, or if the server crashes, redis would remove the keys using it's EXPIRE mechanism.

@darrachequesne
Copy link
Member

Closed by #109

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.