Server is not cleaning up namespaces after client is rejected in middleware.

[carera]

Hello team! 💜

Describe the bug
When using SocketIO with adapter (namely redis adapter in my case) and dynamic namespaces, the namespace isn't cleaned up when SocketIO server throws an error in middleware.

I believe this is a continuation of a fix @stevebaum23 previously made here: #4602
The new flag cleanupEmptyChildNamespaces is cleaning up namespaces when last client disconnects gracefully. It is however not cleaning up properly when client is rejected in middleware.

To Reproduce

Socket.IO server version: 4.7.1

Server

import { Server } from "socket.io";
import Redis from "ioredis";
import { RedisAdapter } from "@socket.io/redis-adapter";

const pubClient = new Redis({
  host: "localhost",
  port: 6370,
});
const subClient = pubClient.duplicate();

function redisAdapterFactory(nsp) {
  return new RedisAdapter(nsp, pubClient, subClient);
}

const io = new Server({
  transports: ["websocket"],
  adapter: redisAdapterFactory,
  cleanupEmptyChildNamespaces: true,
});

const ns = io.of(/\/path.*/);

ns.use((fn, next) => {
  if (Math.random() > 0.5) {
    next(new Error("not allowed!"));
  } else {
    next();
  }
});

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

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

io.listen(3000);

Socket.IO client version: 4.7.1

Client

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

const socket = io("ws://localhost:3000/path/abc", {
  transports: ["websocket"],
});

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

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

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

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

Expected behavior

In the server code, I have a middleware that allows the client to connect 50% of the time (at random). The client's only job is to connect and disconnect straight away.
When observed on redis (using e.g. redis-cli and MONITOR command), when client connects and gracefully disconnects, the Redis namespace is cleaned up (we observe subscribe and unsubscribe events):

"psubscribe" "socket.io#/path/abc#*"
"subscribe" "socket.io-request#/path/abc#" "socket.io-response#/path/abc#" "socket.io-response#/path/abc#jhkYJI#"
"punsubscribe" "socket.io#/path/abc#*"
"unsubscribe" "socket.io-request#/path/abc#" "socket.io-response#/path/abc#" "socket.io-response#/path/abc#jhkYJI#"

when however the server rejects the client in middleware using next(new Error('not allowed!')), the redis subscription is made, but is never cleaned up:

"psubscribe" "socket.io#/path/abc#*"
"subscribe" "socket.io-request#/path/abc#" "socket.io-response#/path/abc#" "socket.io-response#/path/abc#sZeoKO#"

What seems to be happening is that when we call next(new Error('not allowed!') in a socketIO middleware (as suggested in docs), socketIO lib calls _cleanup on error here
Note that, at this point, redis-adapter is already subscribed to topics in Redis. Thus I believe a cleanup similar to the one in _onclose should happen.

Are you able to assist, please?

Platform:

• Device: MacBook Pro
• OS: MacOS Ventura 13.4.1

Additional context
Add any other context about the problem here.

[darrachequesne]

This is indeed a bug, thanks for the detailed report 👍

[carera]

@darrachequesne thank you for your response. If you are able to nudge me in the right direction, I am more than happy to open a PR. I tried something like this, but that seems to make some tests unhappy:

diff --git a/lib/socket.ts b/lib/socket.ts
index 89b5eea..f520605 100644
--- a/lib/socket.ts
+++ b/lib/socket.ts
@@ -758,7 +758,6 @@ export class Socket<
     }
 
     this._cleanup();
-    this.nsp._remove(this);
     this.client._remove(this);
     this.connected = false;
     this.emitReserved("disconnect", reason, description);
@@ -772,6 +771,7 @@ export class Socket<
    */
   _cleanup() {
     this.leaveAll();
+    this.nsp._remove(this);
     this.join = noop;
   }

[carera]

Took a jab at this #4773

[darrachequesne]

This should be fixed by 0731c0d, included in version 4.7.2.

Please reopen if needed.