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

server.keepAliveTimeout is not applied to sockets that are never used. #23981

Closed
asyncmax opened this issue Oct 30, 2018 · 12 comments
Closed

server.keepAliveTimeout is not applied to sockets that are never used. #23981

asyncmax opened this issue Oct 30, 2018 · 12 comments
Assignees
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem. security Issues and PRs related to security.

Comments

@asyncmax
Copy link

  • Version: v8.11.4
  • Platform: Windws 64-bit
  • Subsystem: http & https

Currently, server.keepAliveTimeout is applied to a socket only after the first response is sent out by the server. If a client connects to the server but does not send any requests, the idle socket is kept open until server.timeout is reached (120 seconds by default).

My understanding is that server.keepAliveTimeout was added in Node v8.0.0 to reduce the number of unused idle sockets and in turn to reduce the overhead of the server. But unused sockets can still hang around. We can easily observe this behavior in Chrome which opens sockets that are never used but kept alive for a long time.

I believe that a socket which has never been used should be treated the same as a keepAlive socket awaiting the next request and should be closed after server.keepAliveTimeout elapses.

@sam-github
Copy link
Contributor

We can easily observe this behavior in Chrome which opens sockets that are never used but kept alive for a long time.

By "long time" you mean 120 seconds, the server.timeout, or longer than that?

@asyncmax
Copy link
Author

Yes, 120 seconds, default value of server.timeout is applied.

@sam-github sam-github added http Issues or PRs related to the http subsystem. security Issues and PRs related to security. labels Oct 31, 2018
@sam-github
Copy link
Contributor

@mcollina has a change on the way that I think will address this for you. Its part of some other work, so it'll be a while before it is ready for a PR. I'll link to it when it is.

@mcollina
Copy link
Member

mcollina commented Nov 1, 2018

I don't think my change will cover this.

@sam-github sam-github self-assigned this Nov 1, 2018
@sam-github
Copy link
Contributor

OK, I'll take a look at it.

@sam-github
Copy link
Contributor

@asyncmax Btw, have you considered doing server.timeout = server.keepAliveTimeout? That would seem to work for you. We might end up with a timeout after initial connection, a timeout for the headers, a timeout for the body, and a timeout for in between requests (aka keep alive), but for now I think you can use the idle timeout to get these unused connections closed more quickly.

@asyncmax
Copy link
Author

asyncmax commented Nov 1, 2018

If we are looking for a workaround, we will be able to come up with some clever ways. Your suggestion can be certainly one of them (default timeout value of 5 seconds for server.keepAliveTimeout seems too tight for active sockets though).

The following snippet is how I workaround this problem for now.

server.on("connection", socket => {
  if (server.keepAliveTimeout)
    socket.setTimeout(server.keepAliveTimeout);
});

server.on("request", req => {
  req.setTimeout(server.timeout || 0);
});

However, I reported this issue because I thought this should be addressed in the Node core. In my opinion, sockets that are in their initial state (never used) are effectively the same as sockets that are in an idle state between requests. I don’t believe we need to introduce another type of timeout.

@Trott
Copy link
Member

Trott commented Nov 25, 2018

@sam-github Are you still looking into this? (Not trying to rush you or anything, but do want to update the assignee and labeling if this is not going moving.)

@sam-github
Copy link
Contributor

Yes, still looking at it.

@underflow00
Copy link

This is still a problem.

@jasnell jasnell added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 26, 2020
@mmomtchev
Copy link
Contributor

I am looking at this and in fact the change is very simple to implement
However, as the keepAliveTimeout is enabled by default, this has the effect of modifying the way some default timeouts work and it will require readjusting a certain number of existing unit tests - async-hooks/test-graph being one of them
Thus, the PR will get treated as a potentially breaking change and it will never make it into the current versions
So maybe introducing a completely new option isn't such a bad idea
Should I submit the PR?

@mcollina
Copy link
Member

mcollina commented Oct 8, 2020

However, I reported this issue because I thought this should be addressed in the Node core. In my opinion, sockets that are in their initial state (never used) are effectively the same as sockets that are in an idle state between requests. I don’t believe we need to introduce another type of timeout.

I think the current behavior is correct. A connection is flagged as keep alive after the first request is received because the client signals it to the server with the Connection: keep-alive header. Node.js applies the keepAliveTimeout when it can assume that it is a keep alive connection.

Note that https://nodejs.org/api/http.html#http_server_requesttimeout should also help.

@mcollina mcollina closed this as completed Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem. security Issues and PRs related to security.
Projects
None yet
Development

No branches or pull requests

7 participants