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

Limited communication between sockets over multiple sails application instances (sails.sockets.*) #3008

Closed
bberry6 opened this issue Jun 9, 2015 · 14 comments
Labels

Comments

@bberry6
Copy link

bberry6 commented Jun 9, 2015

Hello fellow devs!

Similar issues that have been open are #2593 and #849. I am running sails version 0.11.0.

Using socket.io-redis to handle communication between websockets connected to different sails instances is limited to sails.sockets.blast and sails.sockets.broadcast. Using sails.sockets.emit(externalSocketId, 'foo'); will not emit the event foo to the externalSocketId.

Additionally, running sails.sockets.subscribers with a room name that websocket connections from different sails instances have joined will only return the socket ids of the websocket connections that are connected to the instance of the requesting websocket.

i.e.
lets say my socket id is ab12 and I'm connected to an app on port 8000.
another socket id cd34 is connected to app on port 9000.
They both join the room foo.
Running

sails.sockets.subscribers('foo') will only output ['ab12'] instead of ['ab12','cd34'].

I have created a simple test environment to show the issue.

For now, I will be using sails.sockets.broadcast as a work around, where I will broadcast messages to rooms that are the socket ids of the intended recipient.

@nozer
Copy link

nozer commented Jun 9, 2015

+1

@odnarb
Copy link

odnarb commented Jun 10, 2015

+1 I'm running nginx, 4 instances and this results in an empty array on instance-B, if all my socket clients are connected to instance-A. (I'm running a node-inspector session on instance-B):

(where myRoomID was used to subscribe a client to a custom room just as in OP's example)
var socketIDs = sails.sockets.subscribers(myRoomID)

I just happened to see this issue while debugging this morning so I'll look into using sails.sockets.broadcast, but I'm not so sure about the security.

Also this looks similar to #2139 ?

@odnarb
Copy link

odnarb commented Jun 10, 2015

Come to think of it, without support of sails.sockets.subscribers() for a multiple sails.js instance solution, I can't keep track of my connected workers very easily without falling back to tracking that via the db which is "slower" and is what I'm trying to avoid. I need to emit events to specific workers, connected to specific room ID's instead of just blasting to a room full of workers.

Thanks and sorry for double posting, but I thought more insight would help clarify this use case.

@sgress454
Copy link
Member

Ok, after looking into this quite a bit, and with help from @Oldwo1f, I can confirm that there's an issue with .subscribers() when used over multiple nodes, stemming from the fact that each socket.io-redis adapter instance only keeps track of its own room subscribers. As far as I can tell there's no way to get a list of subscribers to a channel directly from Redis, and even if there was it probably wouldn't be trivial to associate those subscribers with socket IDs. The biggest impact of this on Sails is that the internal method that "introduces" clients to new model instances when they are created doesn't work in a clustered environment. I'll be working on a workaround for that.

In the meantime, remember that you always have access to the low-level join and broadcast methods via sails.sockets.*, and you should be able to do nearly anything with those. The only time you really need .subscribers() is when you want to take all sockets in one room, and subscribe them to another room. But there's a workaround there, too: just broadcast a message to all of those sockets telling them to go subscribe themselves to the new room. That may be how we end up implementing .introduce as well, although I'm looking into other options that wouldn't require updating sails.io.js...

@bberry6
Copy link
Author

bberry6 commented Jun 12, 2015

@sgress454

Just wanted to point out that in addition to sails.sockets.subscribers(), the sails.sockets.emit() also will not emit messages to an external websocket connection. Maybe this is also due to the same problem with .subscribers()?

Of course, sails.sockets.broadcast() is also a work around for this, as long as the room being broadcasted to has been joined only by the single websocket connection.

Also, thanks for looking into this, I can imagine that it is a quite complex problem to solve with lots of moving parts.

@sgress454
Copy link
Member

@bberry6 exactly--you can use rooms to get around almost any problem. If you look at socket.io-adapter, which defines the interface that socket.io-redis and any other socket.io adapters use, it doesn't even declare emit as a method; it only cares about handling rooms and broadcasting. Not to speak for @rauchg, but I would guess that emit is always intended for a single node to send a message to one of its connected sockets, and nothing more--which it does just fine. In order for it to speak to a socket on another node, it would have to use some form of broadcast internally anyway.

@bberry6
Copy link
Author

bberry6 commented Jun 12, 2015

@sgress454
Ah, I see, well that makes a lot of sense!

Should that information be documented on the sails.sockets.* reference section of the sailsjs.org site?

@sgress454
Copy link
Member

Ok, pushed some updates to sails and sails-hook-sockets that address this problem by setting up a message bus that all Sails apps subscribe to, and having the pubsub introduce and retire methods use that bus to announce new / destroyed model instances. So, this most directly helps with the problem in #2990, but as per my comments above, that's really the biggest issue. Everything else can be worked around using the low-level .join and .broadcast methods.

If you'd like to try this out, you'll need to use the master branch of Sails and make sure you npm install sails-hook-sockets and get version >= 0.11.23. Lift two Sails apps on different ports using the socket.io-redis adapter to share a socket store, a shared database, and a User model, then open a tab for each app and do the following in each:

  1. io.socket.on("user", console.log.bind(console))
  2. io.socket.get("/user", console.log.bind(console))

Then in one tab do:

io.socket.post("/user", {"name": "joe"}, function(user) {io.socket.put("/user/" + user.id, {name: "bob"}, console.log.bind(console));});

and in the other tab you should see both created and updated messages.

@Oldwo1f has a pre-made testing environment for this here.

.subscribers() will still not give you the list of all sockets subscribed to a room across all nodes, and it probably never will. It's possible that a separate method (.allSubscribers()?) could be implemented using the message bus, but it would involve keeping an updated list of Sails nodes in the cluster. Anyone who feels like tackling that is welcome, but chances are it's not really needed. For reference, you can send a message to the bus using

sails.hooks.sockets.broadcastAdminMessage(event, data)

or

sails.hooks.sockets.blastAdminMessage(event, data)

and listen for those messages using sails.on('hook:sockets:adminMessage')

@Oldwo1f
Copy link

Oldwo1f commented Jun 13, 2015

@sgress454 I doesnt work for me.

Are you sure this work for you?
Whilch version of sails should i have?
~0.11.0 or ~0.12.0-rc1
it doesnt work for each of this 2 version

@sgress454
Copy link
Member

@Oldwo1f You need to install Sails from the master branch on Github. Do:

npm i git://github.com/balderdashy/sails.git

on each app.

@Oldwo1f
Copy link

Oldwo1f commented Jun 14, 2015

Ok it works for me too. for the sample case at least. now i ll update sails in my apps and see if it works

Thanks for fixing this

@Oldwo1f
Copy link

Oldwo1f commented Jun 14, 2015

[comment moved to #3014]

@Oldwo1f
Copy link

Oldwo1f commented Jun 18, 2015

@sgress454 Hi Sgress I would like to let you know thats all is working fine for me now.Also I would like to mention that (because we disscuss about it before): I dont need a socket request to update model in order to let other socket subscribe know about modification. I simple http req works great as long as you use publishUpdate().

@mikermcneil
Copy link
Member

Should that information be documented on the sails.sockets.* reference section of the sailsjs.org site?

Definitely need to do this-- for both .emit() and .subscribers(). I can check with @rauchg on the intended behavior here, but I think this analysis is correct. I believe the resourceful pubsub method docs are fine, but we should definitely give the direct sails.sockets.* methods a thorough pass, determine any that might fail in a multi-node environment, then provide a prominent explanation/warning of that behavior in the documentation for those pages.

To start with, we need to verify:

  • whether or not .emit() in socket.io truly works in a multi-node environment (my understanding presently is that it works sometimes depending on server configuration (see http://socket.io/docs/using-multiple-nodes/#sticky-load-balancing)
    • (and what Guillermo's plans are for it long term)
  • whether or not .subscribers() works in a multi-node environment (pretty sure this one is always a "no" because of how the socket.io redis adapter works)
    • (and what Guillermo's plans are for it long term)

I'm going to open a new issue that points back here from sails-hook-sockets as a way to track the documentation efforts.

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

No branches or pull requests

7 participants