Skip to content

Race condition with dynamic namespaces #4136

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
sebamarynissen opened this issue Oct 22, 2021 · 2 comments
Closed

Race condition with dynamic namespaces #4136

sebamarynissen opened this issue Oct 22, 2021 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@sebamarynissen
Copy link
Contributor

I have noticed that a race condition exists with dynamic namespaces. Consider the following setup

// Server side
import { Server } from 'socket.io';
const io = new Server(3000, {});

io.of(async (nsp, auth, next) => {
  let exists = !!await queryDatabaseForResource({});
  next(null, exists);
});

If the query to the database takes some time and a lot of clients are trying to connect to the same namespace in a short period of time, the namespace will be created multiple times. This will result in sockets being connected to a namespace that is overridden by another one, and hence they will no longer be receiving events.

Looking at Server.prototype._checkNamespace, I think this can be solved as follows:

Server.prototype._checkNamespace = function(name, auth, fn) {
  if (this.parentNsps.size === 0) return fn(false);
  const keysIterator = this.parentNsps.keys();
  const run = () => {
    const nextFn = keysIterator.next();
    if (nextFn.done) return fn(false);
    nextFn.value(name, auth, (err, allow) => {
      if (err || !allow) {
        run();
      } else if (this._nsps.has(name)) {

        // If the namespace has been created in the meantime, do not create it again.
        return fn(this._nsps.get(name));

      } else {

        const namespace = this.parentNsps
          .get(nextFn.value)
          .createChild(name);

        this.sockets.emitReserved('new_namespace', namespace);
        fn(namespace);

      }
    });
  };
  run();
};

I will file a PR to solve this.

Context

The odds of this race condition happening are obviously rather low, but I experienced them on a website of mine where I organize tournaments. If the tournament starts, people are all sent to a socket.io namespace where their match is hosted. These namespaces are created dynamically, and so it happens that a large amount of people tries to connect at the same moment.

@sebamarynissen
Copy link
Contributor Author

PR filed (#4137).

darrachequesne pushed a commit that referenced this issue Oct 24, 2021
Using an async operation with `io.use()` could lead to the creation of
several instances of a same namespace, each of them overriding the
previous one.

Example:

```js
io.use(async (nsp, auth, next) => {
  await anOperationThatTakesSomeTime();
  next();
});
```

Related: #4136
@darrachequesne
Copy link
Member

Good catch! Thanks for the detailed analysis 👍

This should be fixed by 9d86397

@darrachequesne darrachequesne added this to the 4.3.2 milestone Nov 8, 2021
darrachequesne added a commit that referenced this issue Jun 26, 2022
Using an async operation with `io.use()` could lead to the creation of
several instances of a same namespace, each of them overriding the
previous one.

Example:

```js
io.use(async (nsp, auth, next) => {
  await anOperationThatTakesSomeTime();
  next();
});
```

Related: #4136

Backported from 9d86397
dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
Using an async operation with `io.use()` could lead to the creation of
several instances of a same namespace, each of them overriding the
previous one.

Example:

```js
io.use(async (nsp, auth, next) => {
  await anOperationThatTakesSomeTime();
  next();
});
```

Related: socketio#4136
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants