Socket.IO Adapter Close causes premature HTTP Server close() call and blocks application shutdown

[laino]

On Nest application shutdown, close() is called on the socket.io server here.

Internally socket.io will now disconnect all sockets handled by itself, then calls close() on the underlying http server here.

This is a problem because now we are blocking until all connections on the underlying http server are closed, but before the nest application has any chance to run logic that would disconnect other long-running connections. That means that if there's a long running connection that wasn't handled by that socket.io adapter, we now block forever.

For instance this disconnect logic is never reached.

[laino]

As a workaround you can hack in your own logic that disconnects clients and prevents new connections before initiating application shutdown.

[laino]

Here's a workaround:

// FIXME: workaround for https://github.com/nestjs/nest/issues/13910
if (forceCloseConnections) {
    const server = (app as NestExpressApplication).getHttpServer();

    server.close = (close => (cb) => {
        close(cb);
        server.closeAllConnections();
        return server;
    })(server.close.bind(server));
}

[laino]

I think a fix for this is to never call SocketIOServer.close(), and instead track connections ourselves in the websocket adapter, destroy them on close, and also destroy any new incoming connections that may happen after the adapter is stopped, but before nest closes the actual http server.

Another way would be to close the http server early in the shutdown process without waiting for the callback, progress through other shutdown logic, then finally wait for the callback:

1. Call httpServer.close(closeCallback), this will synchronously make it not accept new connections
2. Perform nest application shutdown
3. if forceCloseConnections is set, call destroy() on all remaining sockets
4. wait for closeCallback

With the latter way it is up to each component what it wants to do with in-flight requests during shutdown, but they won't have to worry about new requests coming in.

[kamilmysliwiec]

Would you like to create a PR for this issue? We should only do that for adapters that don't share the HTTP server instance (so for those that use a different port than a host application)

[besrabasant]

// FIXME: workaround for https://github.com/nestjs/nest/issues/13910
if (forceCloseConnections) {
    const server = (app as NestExpressApplication).getHttpServer();

    server.close = (close => (cb) => {
        close(cb);
        server.closeAllConnections();
        return server;
    })(server.close.bind(server));
}

@laino Could you please provide a full example of this?

[sapenlei]

Would you like to create a PR for this issue? We should only do that for adapters that don't share the HTTP server instance (so for those that use a different port than a host application)

I want to create a PR for this.

[kamilmysliwiec]

PRs are more than welcome