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

[jbaudanza]

I ran into this issue as well. At the very least, I think the documentation of .to should be updated to warn people against using this pattern.

[sebamarynissen]

@darrachequesne I saw your fix for this issue with ac9e8ca and just wanted to mention that, as you already pointed out yourself, it is indeed not backwards compatible. In a codebase of mine I used a fragment like this:

for (let id of ids) {
  io.to(id);
}
io.emit('some', 'message');

which will break. I can imagine that I'm not the only one doing something like this, so I think this has to go into a semver major.

[darrachequesne]

This was fixed by ac9e8ca and included in socket.io@4.0.0

Calling io.to() (or any other broadcast modifier) will now return an immutable instance:

const operator1 = io.to("room1");
const operator2 = operator1.to("room2");
const operator3 = socket.broadcast;
const operator4 = socket.to("room3").to("room4");

operator1.emit(/* ... */); // only to clients in "room1"
operator2.emit(/* ... */); // to clients in "room1" or in "room2"
operator3.emit(/* ... */); // to all clients but the sender
operator4.emit(/* ... */); // to clients in "room3" or in "room4" but the sender

Documentation: https://socket.io/docs/v3/migrating-from-3-x-to-4-0/#io-to-is-now-immutable

[chrisdlangton]

I'm here because there is apparently a big bug but after reading I see absolutely no bug at all

What I do see is a very big misunderstanding of a fundamental and simple JavaScript evaluation order. If that evaluation order is taken into consideration the perceived bug turns out to live in the implementer code not socket.io

To be concise, we are talking about 'method chaining'. i.e. obj.chain_func1().chain_func2() where:

• obj is io
• chain_func1 is to
• chain_func2 is emit

The example provided and discussed is io.to('my-private-room').emit('event', data), and the evaluation is as expected:

• 1st .to() will run, and it return an instance to io which then;
• io has .emit() chained and given the input of data that is expressed as a dictionary with an await function as a value - so the await function sleep(100) executed 2nd and it's return value is stored to the dictionary key
• 3rd to execute is .emit() with the value it was given and had executed in the 2nd order

I understand many who claim there is a bug have a perceived race-condition, and having io.to immutable is required to fix that. This evaluation order is extremely obvious to me, and not a race-condition bug. There is nothing that any JavaScript library can do about evaluation order of chaining and inputs.

Maybe they could remove the ability for chaining? That would make the evaluation order of chaining a non-issue - but it seems so obvious (chaining evaluation is basic JavaScript syntax) that I am surprised this issue exists.

I really did not mean this to read like a mad burn to the people who claim there is a bug, I really genuinely intend this explanation to educate and help readers to see that there is no bug here at all.

Tip: you don't understand chaining yet? or the evaluation order confuses you, then it is a good idea to simply avoid chaining and the so-called work around is actually not a work-around in reality, it's just the more explicit version of using socket.io that avoids chaining

EDIT: the change in ac9e8ca and included in socket.io@4.0.0 added a very nice feature, which does not actually change the evaluation order I describe above and there is still a perceived race-condition in the provided example. i.e. sleep will still evaluate after io.to and before io.emit so I'd still not rely on the value of io passed from .to to .emit to be synchronous in sequence.. that's not how JavaScript event loop works.
So now that we have io.to immutable, we are giving .emit a new io object and in practice we have a non-standard approach that looks like chaining, but the instance of io was forked from the original io instance at .to and is no longer the same instance that was chained to .to because .to now returns a fork of it

[Overdash]

@chrisdlangton surely from your example emit() should only send the data out to room specified in to()?
If you agree with that behaviour, then that's where the bug everyone is talking about lies. I.e. it was NOT sending the data to the room specified, but instead to all connections, (essentially a leak). I don't think this behaviour should be expected with chaining - unless to() does not actually filter the connections to be targeted (which, as per the documentation, its assumed it does)

[chrisdlangton]

@Overdash you're still assuming the order is (1) .to() then (2) .emit() which is not the reality.

This new approach in version 4.0.0 will not return the object instance any more, instead it returns a new immutable object copied from the original effectively forking it. The evaluation is still not (1) .to() then (2) .emit() however it mitigates any side effects of the asynchronous execution by breaking away from the JavaScript paradigm of chaining where the object this is returned for chaining.

When you see that there is an await (an asynchronous function is made sequential when you use await) you see that there is an asynchronous action that occurs between .to() and .emit() therefore cpu cycles on the event loop are passing as expected and the this reference passed to .emit() would be computationally accurate when it is executed.
But a developer that ignores the expected side-effects that occur during asynchronous cpu cycles on the event loop, then the current state used by .emit() may be unexpected to them because they are semantically not expecting asynchronous cycles...

You see now?

[tommoor]

What I do see is a very big misunderstanding of a fundamental and simple JavaScript evaluation order.

evaluation order is extremely obvious to me

you don't understand chaining yet? or the evaluation order confuses you, then it is a good idea to simply avoid chaining

You see now?

@chrisdlangton your comments don't read like a "mad burn", but they are extremely condescending. There was a bug, and it's been resolved from what I can tell – your input isn't particularly helpful.

[chrisdlangton]

@tommoor simply put, evaluation order of chaining and their inputs is a fundamental JavaScript concept. Learning this and reasoning with it for any code or library is a basicc skill. The value i provided in explaining it correctly and giving clarity that it's not a 'bug' but perhaps a misunderstanding of the language that can apply in any JavaScript situation that utilise standard chaining (that returns this) the readers will gain knowledge to help them and the library maintainers have a tool to better reason with chaining in future.