-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Comments
@adroste I guess middleware is called before the io.on("connection",(socket)=>{
//Rest code above
socket.on("disconnect", () => {
console.log(`disconnect ${socket.id}`);
});
}) and it should work. |
@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. |
@adroste this behavior is expected. The |
@darrachequesne But then, 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:
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();
});
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
});
}); |
@adroste How can we possibly detect |
@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 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. |
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
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
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
Describe the bug
If I use a middleware (e.g. for authentication) I cannot handle a client disconnect because
disconnect
event-handler does not get executedsocket.connected
won't update properly (fromtrue
tofalse
)To Reproduce
Please fill the following code example:
Socket.IO server version:
4.2.0
Server
Socket.IO client version:
4
Client
Expected behavior
socket.connected
will update properly anddisconnect
event-handler will runPlatform:
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-handlerThe text was updated successfully, but these errors were encountered: