Using await within emit causes payload to be sent to all connected clients

[tommoor]

You want to:

• report a bug
• request a feature

Current behaviour

Using async/await to construct payloads results in a race condition and messages potentially being broadcast to all connections.

Steps to reproduce

Fiddle: https://github.com/tommoor/socket.io-fiddle/tree/async-await-critical-issue
TLDR: Using await within the payload construction for emit will cause the message to be sent to all connections instead of the specific room. eg:

io.to('my-private-room').emit('event', {
  data: await myDatabaseQuery()
});

Expected behaviour

Data should be sent to the correct room and not leaked to all clients.

Setup

• OS: All
• browser: All
• socket.io version: 2.2.0

[quangpl]

Sometime my app has issue :
Cannot read property 'emit' of undefined
A part of my code :

    socket.on('typing', function (data) {
     
            if (data.to) {
                let strangerSocket1 = io.sockets.sockets[data.to];
                socket.emit('typing', data);
                strangerSocket1.emit('typing', data);
            }
    });

What can I do to resolve this problem ?
Thank you !

[Overdash]

Hey, I see two possible problems with your code:

1. I don't believe "strangerSocket1" is defined anywhere? Perhaps you intended to pass it as an argument?
2. Not so much an issue more like bad practice. You shouldn't need to await emit. It uses callbacks to be asynchronous, it doesn't return a Promise (instead it returns an EventEmitter). There's nothing in your async function that actually needs to be awaited :) (since nothing there returns a promise) it can all be called synchronously (and it should) because it'll behave the same way.

To solve this problem you'll simply need to define/grab strangerSocket1 somewhere :).
Hope this helped!

[quangpl]

Hey, I see two possible problems with your code:

1. I don't believe "strangerSocket1" is defined anywhere? Perhaps you intended to pass it as an argument?
2. Not so much an issue more like bad practice. You shouldn't need to await emit. It uses callbacks to be asynchronous, it doesn't return a Promise (instead it returns an EventEmitter). There's nothing in your async function that actually needs to be awaited :) (since nothing there returns a promise) it can all be called synchronously (and it should) because it'll behave the same way.

To solve this problem you'll simply need to define/grab strangerSocket1 somewhere :).
Hope this helped!

Sorry, I have some mistake when upload my code . I have been update it .
And strangerSocket1 has been define be let strangerSocket1 = io.sockets.sockets[data.to];
In detail : It's is socket of free member from array of object .

My issue Cannot read property 'emit' of undefined is sometimes happen not always.
What can I do ? Thanks for your reponse .

[Overdash]

I'm assuming data.to is the socketId of the socket you want to emit to. If that's the case, the correct way to emit from server would be:
io.to(data.to).emit('typing', data) (so strangerSocket1 can be completely removed)

[quangpl]

I'm assuming data.to is the socketId of the socket you want to emit to. If that's the case, the correct way to emit from server would be:
io.to(data.to).emit('typing', data) (so strangerSocket1 can be completely removed)

Thank you ! I think it's a best answer !
Have a nice day :D

[ralphpig]

@tommoor
Not sure why this issue was highjacked but I've been dealing with an issue like this for the past few days.

The problem isn't with async/await directly. Using async/await will work fine as long as you ensure .emit(..) or .clients(..) isn't called elsewhere before the Promise resolves. This is because .to(..) doesn't return a new instance, and .emit(..) and .clients(..) reset Namespace.prototype.rooms, which is set by .to(..).

In your example:

[1, 2, 3].forEach(async () => {
  console.log("sending to private room");
  io.to('my-private-room').emit('event', {
    data: await sleep(100)
  });
});

The call order is:

io.to(roomId)   // rooms: []       -> [roomId]
io.to(roomId)   // rooms: [roomId] -> [roomId]
io.to(roomId)   // rooms: [roomId] -> [roomId]
io.emit(..)     // rooms: [roomId] -> []        Promise resolved
io.emit(..)     // rooms: []       -> []        Promise resolved
io.emit(..)     // rooms: []       -> []        Promise resolved

After the first Promise resolves, io.emit(..) sends to roomId and clears rooms. So subsequents emits send to every socket in the namespace.

The simple fix for this is to move the await before io.to(..) like such:

[1, 2, 3].forEach(async () => {
  console.log("sending to private room");
  let data = await sleep(100);
  io.to('my-private-room').emit('event', {
    data
  });
});

This yields a call order of:

io.to(roomId)   // rooms: []       -> [roomId]  Promise resolved
io.emit(..)     // rooms: [roomId] -> []
io.to(roomId)   // rooms: []       -> [roomId]  Promise resolved
io.emit(..)     // rooms: [roomId] -> []
io.to(roomId)   // rooms: []       -> [roomId]  Promise resolved
io.emit(..)     // rooms: [roomId] -> []

Hope this helps

[tommoor]

Yep, this seems right and how we resolved it. The bug is really bad though and leaked production data to the wrong user in my circumstance so felt it worth filing as a warning to others

[caasi]

@ralphpig: After checking the implementation of to and emit, I think it's unfixable if rooms is mutable when resolving the promise.

[jasonlewicki]

@tommoor right there with you. Leaked data to wrong client. Yikes.

[seifsg]

To get around this for my case, I store socket object in-memory in a map of immutablejs with the roomID as they key. And then I emit directly to the sockets.

[lifenautjoe]

So absolutely no chance of fixing this? Are we stuck on .then.then.then forever?

[ralphpig]

@lifenautjoe Not sure of your exact situation or how your promise chain would look any different considering this issue. But I'm not sure this is something that needs to be "fixed". I think it's just something that requires some special consideration / documentation.

You just have to be careful to resolve your promise before making any calls to .to or .emit.
.emit has to be called synchronously after you call .to.

[darrachequesne]

The behavior is indeed quite surprising and should be fixed IMO. I'm not sure if we could fix this in a backward-compatible way though.

Until then, I added a note here: https://socket.io/docs/v3/rooms/#Usage-with-asynchronous-code