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

socket.join could lead to a room leak. #4380

Closed
baptistejamin opened this issue May 24, 2022 · 6 comments
Closed

socket.join could lead to a room leak. #4380

baptistejamin opened this issue May 24, 2022 · 6 comments
Labels
bug Something isn't working
Milestone

Comments

@baptistejamin
Copy link

Describe the bug

We are using socket io since years now, and we discovered a very weird bug that can occur under some conditions.

It seems it's possible to join rooms using the base adapter, even if the socket is destroyed or disconnected. I don't know if it was intended initially, but adapter.addAll shouldn't be connected if the socket is no longer available.

A such issue could appear for instance under an elevated database latency. If the client socket disconnected before the socket.join action, the join action is performed anyway.

To Reproduce

  1. Use the client
  2. Leave the page before 15s

Run those steps 5 times.

DUMMY_ROOM should have 5 different sockets attached to the rooms, even if those sockets are no longer connected.

Please fill the following code example:

Socket.IO server version: any version

Server

import { Server } from "socket.io";

const io = new Server(3000, {});

io.on("connection", (socket) => {
  socket.on("ping", () => {
    // Simulate a database latency
    new Promise((resolve) => {
      setTimeout(resolve, 15000);
    })
    .then(() => {
      socket.join("DUMMY_ROOM");
    });
  })

  socket.on("disconnect", () => {
    console.log(`disconnect ${socket.id}`);
  });
});

Socket.IO client version: x.y.z

Client

import { io } from "socket.io-client";

const socket = io("ws://localhost:3000/", {});

socket.on("connect", () => {
  socket.emit("ping")
});

socket.on("disconnect", () => {
  console.log("disconnect");
});

Expected behavior

socket.join shouldn't propagate this.adapter.addAll if the socket no longer exists.

@baptistejamin baptistejamin added the to triage Waiting to be triaged by a member of the team label May 24, 2022
@darrachequesne
Copy link
Member

I could indeed reproduce, thanks 👍

It can also happen within a middleware:

io.use((socket, next) => {
  socket.join("DUMMY_ROOM");
  next(new Error("nope"));

  console.log(io.of("/").adapter.rooms); Map(1) { 'DUMMY_ROOM' => Set(1) { 'pqUiVlJ7Q6juccjiAAAB' } }
});

This could explain issues like #4067

Let's fix that.

@darrachequesne darrachequesne added bug Something isn't working and removed to triage Waiting to be triaged by a member of the team labels May 25, 2022
darrachequesne added a commit that referenced this issue May 25, 2022
Calling `socket.join()` after disconnection would lead to a memory
leak, because the room was never removed from the memory:

```js
io.on("connection", (socket) => {
  socket.disconnect();
  socket.join("room1"); // leak
});
```

Related:

- #4067
- #4380
@darrachequesne
Copy link
Member

OK, so this should be fixed by 18f3fda I think.

darrachequesne added a commit that referenced this issue Jun 26, 2022
Calling `socket.join()` after disconnection would lead to a memory
leak, because the room was never removed from the memory:

```js
io.on("connection", (socket) => {
  socket.disconnect();
  socket.join("room1"); // leak
});
```

Related:

- #4067
- #4380

Backported from 18f3fda
@HsinHeng
Copy link

HsinHeng commented Jul 1, 2022

Thanks for to fix it.
Does it plan to release at next version ?

@jiouiuw
Copy link

jiouiuw commented Sep 4, 2022

@HsinHeng released.

this issue should be closed

@HsinHeng
Copy link

HsinHeng commented Sep 4, 2022

@HsinHeng released.

this issue should be closed

Thanks for your notified.

@darrachequesne
Copy link
Member

This should indeed be fixed by 18f3fda, included in version 4.5.2 (and backported in the 2.x branch: f223178).

Thanks for the clear and detailed report ❤️

Please reopen if needed.

@darrachequesne darrachequesne added this to the 4.5.2 milestone Sep 4, 2022
dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
Calling `socket.join()` after disconnection would lead to a memory
leak, because the room was never removed from the memory:

```js
io.on("connection", (socket) => {
  socket.disconnect();
  socket.join("room1"); // leak
});
```

Related:

- socketio#4067
- socketio#4380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants