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

Fix bug caused by EventEmitter placement #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Soulike
Copy link

@Soulike Soulike commented Oct 29, 2021

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!

@Soulike Soulike changed the title Bug caused by EventEmitter placement Fix bug caused by EventEmitter placement Oct 29, 2021
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.

None yet

1 participant