Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

us_listen_socket_close is suppose to close all connections or just http #663

Closed
jcherrabi opened this issue Dec 16, 2021 · 7 comments
Closed

Comments

@jcherrabi
Copy link

Hello everyone,
when i call us_listen_socket_close it seems to close the http/https socket but not the websockets sonnections, is this the correct behavior ? and i should handle closing down the websockets separately?

thanks,
Jay

@e3dio
Copy link
Contributor

e3dio commented Dec 16, 2021

us_listen_socket_close closes the listen socket for new http/ws connections, existing connections for http and ws are still open. res.close() and ws.close() force close connections, or .end() graceful close. If you want Node to exit naturally you need to wait or close them yourself, but it's not required you can tell Node to exit and they will close

@jcherrabi
Copy link
Author

jcherrabi commented Dec 17, 2021

Hello @e3dio
Thanks for your reply here is the test i did and socket is still listening to WS but not Http...
my listening code is:

let token = any;
}).listen(port, (token: any) => {
    /* Save the listen socket for later shut down */
    listenSocket = token;
    if (token) {
        console.log('Listening to port ' + port, token);
    } else {
        console.log('Failed to listen to port ' + port);
    }
});

and to shutdown i used this simple get:

.get('/shutdown', (res, req) => {
    if (listenSocket) {
        res.end('Okay, shutting down http server only now! websockets still running');
        /* This function is provided directly by µSockets */
        uWS.us_listen_socket_close(listenSocket);
        listenSocket = null;
    } else {
        /* We just refuse if alrady shutting down */
        res.close();
    }

after calling shutdown on that token i can no longer reach http which is expected but i can still connect to ws socket . is that expected?

.ws('/*', {
    /* Options */
    compression: uWS.SHARED_COMPRESSOR,
    maxPayloadLength: 16 * 1024 * 1024,
    idleTimeout: 10,
    upgrade: (res: HttpResponse, req: HttpRequest, context: us_socket_context_t) => {
        console.log('An Http connection wants to become WebSocket, URL: ' + req.getUrl() + '!');
        /* Can't return or yield from here without responding or attaching an abort handler */
        res.onAborted(() => {
            res.aborted = true;
        });
        /* This immediately calls open handler, you must not use res after this call */
        res.upgrade({
            url: req.getUrl(),
            key: req.getHeader('sec-websocket-key'),
            protocol: req.getHeader('sec-websocket-protocol'),
            extensions: req.getHeader('sec-websocket-extensions'),
        },
            /* Spell these correctly */
            req.getHeader('sec-websocket-key'),
            req.getHeader('sec-websocket-protocol'),
            req.getHeader('sec-websocket-extensions'),
            context);
    },
    /* Handlers */
    open: (ws: uWS.WebSocket) => {
        console.log('A WebSocket connected!');
    },

@e3dio
Copy link
Contributor

e3dio commented Dec 17, 2021

@jcherrabi I don't see you closing any websockets in your shutdown code, that would probably fix your issue #575 us_listen_socket_close stops new connections

@davidmurdoch
Copy link
Contributor

closes the listen socket for new http/ws connections, existing connections for http and ws are still open

Just to expand on what is being said here for HTTP connections: in the case of HTTP connections that are using Keep Alive (sending a Connection: Keep Alive header) us_listen_socket_close will not prevent new requests from being processed and the amount of time the connection remains open is up to the client that sent the request.

This means that uWS cannot gracefully close a long-lived HTTP connection later like it can with a WS connection.

The following code will cause the process to run indefinitely, even after sending SIGINT (Ctrl + C) to the server, as the client keeps the connection open even after us_listen_socket_close as closed. There is no way for the server to keep track of the open HTTP connection to gracefully dispose of them.

// server.js
// run as `node server`
const {App, us_listen_socket_close} = require('uWebSockets.js');

let _listenSocket = null;
App().any("/*", (res) => {
  res.end("hello"); // don't send `end(_, true)` because we _want_ to allow keep alive connections.
}).listen(5555, (listenSocket) => {
  _listenSocket = listenSocket;
  console.log("listening...");
});

process.on("SIGINT", () => {
  us_listen_socket_close(_listenSocket)
  _listenSocket = null;
  console.log("Listen socket closed, waiting for process to exit...");
});
// client.js
// run as `node client`
const http = require("http");

// node's default is to disable keepAlive so we use our own Agent for these tests
const agent = new http.Agent({
  keepAlive: true,
  keepAliveMsecs: 99999
});

const send = () => {
  http.get("http://localhost:5555", { agent }, (res) => {
    // keep hitting the server once every 5 seconds forever
    res.on('end', () => { setTimeout(send, 5000); });
  });
}
send();

I couldn't find a way to work around this that doesn't involve process.exit(). I don't want to process.exit() because in my actual application lots of other libraries may be performing their own clean up, and I don't want to hide potential shutdown errors by forcing the process to exit.

For WS connections it's easy, as we already have to keep track of those, so on shutdown we can iterate over them and close them all. For HTTP I don't see how it is possible.

@e3dio
Copy link
Contributor

e3dio commented Feb 1, 2022

If you add if (!listenSocket) return res.close() at the top of your routes/middleware the next time the client sends request it will close the connection

@davidmurdoch
Copy link
Contributor

if (!listenSocket) return res.close() isn't quite enough, because in my case I'd like to shut it down gracefully on my terms, not the client's. It could be several minutes before the client sends another request.

@e3dio
Copy link
Contributor

e3dio commented Feb 1, 2022

Http timeout is 10 seconds, is not determined by what client sends in header, might be short enough for you https://github.com/uNetworking/uWebSockets/blob/master/src/HttpContext.h#L44

@uNetworking uNetworking locked and limited conversation to collaborators Oct 20, 2022
@uNetworkingAB uNetworkingAB converted this issue into discussion #819 Oct 20, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants