socket.io-adapter
socket.io-adapter copied to clipboard
Fix bug caused by EventEmitter placement
I think the reason for the bug described in #74 is that the EventEmitter calls all listeners synchronously.
The EventEmitter calls all listeners synchronously in the order in which they were registered.
So at runtime, the testcase
adapter.on("leave-room", (room, sid) =>
{
adapter.del("s1", "r1");
});
adapter.addAll("s1", new Set(["r1"]));
adapter.del("s1", "r1");
makes the code of _del()
equivalents to
_del(room, id) {
if (this.rooms.has(room)) {
const deleted = this.rooms.get(room).delete(id);
if (deleted) {
((room, sid) =>
{
adapter.del("s1", "r1"); // 1. does this.rooms.delete(room), and returns;
})()
}
// 2. After the listener execution, this.rooms.get(room) === undefined
if (this.rooms.get(room).size === 0) {
this.rooms.delete(room);
this.emit("delete-room", room);
}
}
}
Therefore I think the bug actually does not involves any asynchronously invoked function calls. The listeners of leave-room
are always executed synchronously before the following codes in _del()
.
If I modify the testcase to
adapter.on("leave-room", (room, sid) =>
{
// makes the call asynchronous
setImmediate(() => {
adapter.del("s1", "r1");
});
});
adapter.addAll("s1", new Set(["r1"]));
adapter.del("s1", "r1");
the error will never happen again.
What I am concerning is that event listeners can modify the resources used by following codes and makes them buggy.
For the fixed version of _del()
_del(room: Room, id: SocketId) {
const _room = this.rooms.get(room);
if (_room != null) {
const deleted = _room.delete(id);
if (deleted) {
// Listeners here can modify the resources used by following codes
this.emit("leave-room", room, id);
}
if (_room.size === 0 && this.rooms.delete(room)) {
this.emit("delete-room", room);
}
}
}
I suggest that the event listeners should be emitted after all resource-related jobs are done, like
_del(room: Room, id: SocketId) {
const _room = this.rooms.get(room);
if (_room != null) {
// all resource-related jobs are guaranteed not to be disturbed
const deletedSocketIdFromRoom = _room.delete(id);
let deletedRoom = false;
if (_room.size === 0 && this.rooms.delete(room)) {
roomDeleted = true;
}
if (deletedSocketIdFromRoom) {
this.emit("leave-room", room, id);
}
if (deletedRoom) {
this.emit("delete-room", room);
}
}
}
The same principle also applies to the error caused by the following test case I found
const adapter = new Adapter({server: {encoder: null}});
adapter.on('create-room', () =>
{
adapter.del('s1', 'r1');
});
adapter.addAll('s1', new Set(['r1']));
/*
if (!this.rooms.get(room).has(id)) {
^
TypeError: Cannot read properties of undefined (reading 'has')
*/
I edited the code and made this pr to solve the problem. Thank you!