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

Disconnect handler not executed if connection drops during middleware execution #4129

Closed
adroste opened this issue Oct 16, 2021 · 8 comments
Closed
Labels
bug Something isn't working
Milestone

Comments

@adroste
Copy link

adroste commented Oct 16, 2021

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

@adroste adroste added the bug Something isn't working label Oct 16, 2021
@kanhaiya-2000
Copy link

kanhaiya-2000 commented Oct 21, 2021

@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
Copy link
Author

adroste commented Oct 21, 2021

@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
Copy link
Member

@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
Copy link
Author

adroste commented Oct 21, 2021

@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
Copy link
Member

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
Copy link

kkleap commented Oct 21, 2021

@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
Copy link
Author

adroste commented Oct 21, 2021

@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 added a commit that referenced this issue Nov 12, 2021
The Socket instance is only considered connected when the "connection"
event is emitted, and not during the middleware(s) execution.

```js
io.use((socket, next) => {
  console.log(socket.connected); // prints "false"
  next();
});

io.on("connection", (socket) => {
  console.log(socket.connected); // prints "true"
});
```

Related: #4129
darrachequesne added a commit that referenced this issue Jun 26, 2022
The Socket instance is only considered connected when the "connection"
event is emitted, and not during the middleware(s) execution.

```js
io.use((socket, next) => {
  console.log(socket.connected); // prints "false"
  next();
});

io.on("connection", (socket) => {
  console.log(socket.connected); // prints "true"
});
```

Related: #4129

Backported from 02b0f73
dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
The Socket instance is only considered connected when the "connection"
event is emitted, and not during the middleware(s) execution.

```js
io.use((socket, next) => {
  console.log(socket.connected); // prints "false"
  next();
});

io.on("connection", (socket) => {
  console.log(socket.connected); // prints "true"
});
```

Related: socketio#4129
@darrachequesne
Copy link
Member

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.

@darrachequesne darrachequesne added this to the 4.4.0 milestone Jun 19, 2023
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