Disconnect handler not executed if connection drops during middleware execution

[adroste]

Describe the bug
If I use a middleware (e.g. for authentication) I cannot handle a client disconnect because

1. disconnect event-handler does not get executed
2. socket.connected won't update properly (from true to false)

To Reproduce

Please fill the following code example:

Socket.IO server version: 4.2.0

Server

import { Server } from "socket.io";

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

io.use(async (socket, next) => {
    console.log(`middleware`, socket.id);

    // bug 1: disconnect handler will never execute
    socket.on("disconnect", () => {
      console.log(`disconnect ${socket.id}`);
    });
    
    // long running async operation
    // client will disconnect before operation has finished
    await new Promise(resolve => setTimeout(resolve, 3000));

    // bug 2: connected state is still true
    console.log(`still connected: ${socket.connected}`);

    next();
});

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

Socket.IO client version: 4

Client

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

const socket = io("ws://localhost:3000/", {});
// force disconnect before long running operation has finished
setTimeout(window.location.reload, 1000);

Expected behavior
socket.connected will update properly and disconnect event-handler will run

Platform:
node.js 16 on macOS

Additional context
The workaround to the problem is: don't trigger side-effects in middleware and always add custom properties to the socket object to transfer information over to connection event-handler

[kanhaiya-2000]

@adroste I guess middleware is called before the connection event is fired. You can move your disconnect event logic into the block

io.on("connection",(socket)=>{
  //Rest code above
   socket.on("disconnect", () => {
      console.log(`disconnect ${socket.id}`);
   });
   
})

and it should work.

[adroste]

@kanhaiya-2000 Of course, the middleware is called before the connection event is fired. Otherwise you wouldn't be able to handle something like authentication upfront.

You cannot move the disconnect register to the connection handler because the connection handler does not fire if the socket connection drops before all registered middlewares have finished. Therefore the disconnect handler wouldn't even get registered in your example.

[darrachequesne]

@adroste this behavior is expected. The socket is not considered connected when the middleware gets executed, it will once the last middleware of the chain calls next().

[adroste]

@darrachequesne But then, the socket.connected property shouldn't be true when the middleware is called.
Nevertheless, it is not a definition problem. The problem here is: how to handle or detect a disconnect (for cleanup etc.) during middleware execution.

[darrachequesne]

the socket.connected property shouldn't be true when the middleware is called

Hmm, yes, totally, you are right, we need to change that.

The connection can indeed be closed during the middleware execution (here).

Two possibilities:

• you listen for the "close" event on the underlying connection:

io.use(async (socket, next) => {
    let closed = false;

    socket.conn.on("close", () => {
        closed = true;
    });
    
    // long running async operation
    // client will disconnect before operation has finished
    await new Promise(resolve => setTimeout(resolve, 3000));

    if (closed) {
        // cleanup
        return;
    }
    next();
});

• or you move your long running operation in the "connection" handler:

io.on("connection", async (socket) => {
    // long running async operation
    // client will disconnect before operation has finished
    await new Promise(resolve => setTimeout(resolve, 3000));

    socket.on("disconnect", () => {
        // cleanup
    });
});

[kkleap]

@darrachequesne But then, the socket.connected property shouldn't be true when the middleware is called. Nevertheless, it is not a definition problem. The problem here is: how to handle or detect a disconnect (for cleanup etc.) during middleware execution.

@adroste How can we possibly detect disconnect event when the socket connection was not fired? socket.connected is true,alright,it may be a problem but why would we need to call disconnect before the connection?

[adroste]

@kkleap If you use the middleware to trigger a side effect, you sometimes need to trigger a cleanup for that effect after the client has disconnected.

Proper example:

Server

import { Server } from "socket.io";

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

const sessions = {};

io.use(async (socket, next) => {

    const authToken = socket.handshake.auth.token;
    // long running async operation
    // client will disconnect before operation has finished
    const user = await authenticate(authToken);

    if (socket.connected) {
        sessions[socket.id] = { socket, user };
    }

    next();
});

io.on("connection", (socket) => {
    socket.on("disconnect", () => {
        delete sessions[socket.id];
    });
});

This implementation will leave the sessions object in a corrupt state if the client disconnects early, because socket.connected is always true and the disconnect handler will not register.

Even if you "lift" the register of the disconnect handler like so:

import { Server } from "socket.io";

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

const sessions = {};

io.use(async (socket, next) => {
    socket.on("disconnect", () => {
        delete sessions[socket.id];
    });

    const authToken = socket.handshake.auth.token;
    // long running async operation
    // client will disconnect before operation has finished
    const user = await authenticate(authToken);

    if (socket.connected) {
        sessions[socket.id] = { socket, user };
    }

    next();
});

The sessions object will still end up corrupted cause the disconnect handler won't fire.

So please fix the implementation to either properly update the connected property (would be my favorite) or just trigger the disconnect handler.

Edit: For the sake of completeness. Yes, you could circumvent the problem in the example by doing something like this:

import { Server } from "socket.io";

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

function getSessions() {
    return Array.from(io.socket.sockets.values());
}

io.use(async (socket, next) => {
    const authToken = socket.handshake.auth.token;
    const user = await authenticate(authToken);
    socket.user = user;
    next();
});

However, it feels wrong to rely on patching objects you don't "own". Also this won't help if you need to track clients that are in a pending state for instance. (E.g. 2FA)

Edit2: @darrachequesne listening for the "close" event on the underlying connection seems to be a great solution. Thanks for the contribution. Nevertheless, I still think that the socket.connected property should be properly updated and set to false on early disconnect or is not set to true before the "connection" event is fired.

[darrachequesne]

Nevertheless, I still think that the socket.connected property should be properly updated and set to false on early disconnect or is not set to true before the "connection" event is fired.

This was fixed in 02b0f73, included in version 4.4.0.

Please reopen if needed.