socket.io icon indicating copy to clipboard operation
socket.io copied to clipboard

Errors thrown by middlewares do not get captured or reported

Open knackstedt opened this issue 11 months ago • 1 comments

Describe the bug If an unexpected error is thrown inside of a middleware, the error is completely ignored and not logged to any output stream or file. This makes errors impossible to debug without adding breakpoints into the dependency. This is obviously bad practice for a production system as rare errors will all get swept under the rug.

See here: https://github.com/socketio/socket.io/blob/7427109658591e7ce677a183a664d1f5327f37ea/packages/engine.io/lib/server.ts#L772

To Reproduce

Please fill the following code example:

Socket.IO server version: 4.8.x

Server

import { Server } from "socket.io";

const io = new Server(3000, {});
io.engine.use((req, res, next) => {
    if (!req.session?.hasSocketAccess) {
        return next(new Error("You do not have access"));
    }
    next();
});

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

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

Socket.IO client version: 4.8.x

Client

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

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

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

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

Expected behavior When the error is thrown in the middleware, it is able to be logged in some manner. Given that the current behavior tries to respond to the open HTTP connection and throws (yet another) error "Cannot set headers after they are sent to the client", I think socket.io needs to support being provided an error logger and if not provided, automatically log such errors to console.error.

Platform:

  • Device: Irrelevant.
  • OS: Irrelevant.

Additional context I would have made a PR to resolve this, but I'm sure the current maintainers would have a preferred implementation for satisfying this need. The underlying "Cannot set headers after they are sent" error is probably an artifact of the implementation here, which might be resolved anyway by adding this logging support. I'm not totally sure what the intention is for that specific method, so I'll leave that up to the maintainers.

knackstedt avatar Dec 17 '24 14:12 knackstedt

Edit: Also affects this closure. https://github.com/socketio/socket.io/blob/7427109658591e7ce677a183a664d1f5327f37ea/packages/engine.io/lib/server.ts#L815 (GitHub is currently quite laggy so I can't modify the original post :/ )

knackstedt avatar Dec 17 '24 14:12 knackstedt