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

http: added closeAllConnections and closeIdleConnections to http.server #42812

Closed
wants to merge 6 commits into from

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented Apr 21, 2022

This PR introduces two new methods on http.Server and https.Server:

  1. closeAllConnections(): Closes all sockets connected to the server.
  2. closeIdleConnections(): Closes all sockets connected to the server which are not actively sending a request or waiting for a response.

Fixes #41578.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 21, 2022
doc/api/https.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@lpinca
Copy link
Member

lpinca commented Apr 21, 2022

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

lib/_http_server.js Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Apr 21, 2022

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

@ShogunPanda
Copy link
Contributor Author

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

Well, the connections list is not exposed to the developer, so they can't really do that.

I agree that all the HTTP* servers should behave the same (and it could come in a future PR for Http2Server), while net.Server and tls.Server serve a different purpose so API parity I don't think is applicable here.

@ShogunPanda
Copy link
Contributor Author

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

Actually it is bug in the implementation - Once non-idle requests have been served, they have to be evicted as well. Thanks for noticing it, I'll update this shortly.

lib/_http_server.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Apr 21, 2022

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

@ShogunPanda
Copy link
Contributor Author

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

I see, make sense. I thought you were talking about the new data structure I recently introduced.

@mcollina
Copy link
Member

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

@lpinca
Copy link
Member

lpinca commented Apr 21, 2022

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

I'm interested because it only takes a set of references to already existing objects. I would be surprised if that turns out to be a major overhead.

@jsumners
Copy link
Contributor

I believe this PR is a direct result of:

The primary issue that impacts performance is retaining references to the requests associated with the sockets. Ideally, this feature would let any active requests finish and then reap the remaining sockets. Currently, Node waits for all sockets to close before terminating even if there are no pending requests on those sockets.

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests. The force close implementation in Fastify ignores this and terminates all sockets regardless of whether or not any requests are currently active. This is a poor implementation, but was the only remotely performant way to get it done.

doc/api/http.md Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Apr 21, 2022

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests.

I understand but in this case the patch would only be useful if the force option is set to 'keep-alive' and there is no guarantee that the process will exit because there might be other open connections like WebSocket connections. The other option value would be equivalent to something like this:

const http = require('http');

const sockets = new Set();

const server = http.createServer();

server.on('connection', function (socket) {
  sockets.add(socket);
  socket.on('close', socketOnClose);
});

function socketOnClose() {
  sockets.delete(this);
}

server.close();

for (const socket of sockets) {
  socket.destroy();
}

@ShogunPanda
Copy link
Contributor Author

@lpinca Yes, for the all case you are right. Even though taking advantage of the node internal is obviously faster than using EventEmitter and introduces less overhead.

@ShogunPanda
Copy link
Contributor Author

About the WebSocket objections, you are also right. But we assume developers know what they are doing :)

The keep-alive option is useful to let current pending request to be handled but without accepting any new request on already opened connections (which is possible as of today).

@ShogunPanda
Copy link
Contributor Author

As a different solution to achieve this, I might leave close as it is now and add the following new methods to http.Server (and https.Server):

  1. server.closeConnections([type]), which performs the same new logic I just added to .close
  2. connections, activeConnections, idleConnectins and expiredCollections, which make the various connections list readable from the developer.

The reason for 1 is to being able to solve the issue without changing existing functionality.

The reason for 2 is to enable developer to do custom connections handling without having to trace them via EE.

What do you think?

@mcollina
Copy link
Member

Will exposing the connection lists be enough to implement this feature in userland?

@ShogunPanda
Copy link
Contributor Author

Absolutely. The other method would just become a fancy for loop.

@mcollina
Copy link
Member

Then let's expose the lists.

@ShogunPanda
Copy link
Contributor Author

Done.

@nodejs/http @lpinca Can you please re-review?

doc/api/http.md Outdated
@@ -1443,6 +1443,31 @@ This event is guaranteed to be passed an instance of the {net.Socket} class,
a subclass of {stream.Duplex}, unless the user specifies a socket
type other than {net.Socket}.

### `server.activeConnections()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A request is not a connection. A request is a communication across a connection. I think server.activeConnections is misleading. According to the subsequent docs, it is server.activeRequests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's connections. I return the sockets, even though the way the sockets are categorized is given by whether there is an active/expired request being sent through it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this help with draining connections? The socket does not expose the requests associated with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point now.
Let me clarify that, of all these methods, only allConnections and idleConnections are strictly needed to properly implement the forced close logic.

But I do see your point and, on a second thought, it makes sense to expose the requests rather than socket. This way the developer has all the needed informations to implement any logic they might want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that requests are typically short lived. Whereas connections can be open for literal wall clock minutes. What Node.js currently supports is:

  • .close stops listening for new connections but waits for current connections to end before the process can exit. This scenario allows new requests on existing connections until they timeout.

There are two other scenarios that can/should be supported:

  1. Terminate all connections regardless of pending/active requests such that the process can terminate. This is the scenario Fastify is currently implementing.
  2. Stop listening for new connections and stop accepting new requests while allowing current requests to finish and subsequently terminating the associated connections.

We have seen other communities provide scenario 1, so maybe 2 isn't feasible. I don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2 is hard as it needs a check every time a request is finished. Which will impact performance. So eventually (which is the intention of this PR) we go with 1 (as suggested in my last comment).

doc/api/http.md Outdated
@@ -1453,6 +1478,18 @@ added: v0.1.90

Stops the server from accepting new connections. See [`net.Server.close()`][].

### `server.expiredConnections()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear what this provides. If a request is expired, that means Node is no longer tracking it, correct? Similarly, if a connection has expired it should have also been removed by Node, yes? So what utility does this method provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are 90% right. The thing is that the Node evicts expired connection every http.Server.connectionsCheckingInterval milliseconds (default is 30s), so it can definitely happen that connections are expired but not evicted yet. Hence this method to eventually allow the user to do other things (like log these connections before evictions or similar).

doc/api/http.md Outdated Show resolved Hide resolved
lib/_http_server.js Outdated Show resolved Hide resolved
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing potentially large lists of resources generally feels like an anti-pattern in Node.js, and unless the application code is entirely synchronous, it is prone to race conditions.

doc/api/http.md Outdated Show resolved Hide resolved
@ShogunPanda ShogunPanda deleted the close-all-connections branch May 3, 2022 12:04
RafaelGSS pushed a commit that referenced this pull request May 10, 2022
Fixes: #41578

PR-URL: #42812
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS added a commit that referenced this pull request May 10, 2022
Notable changes:

* Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812

PR-URL: TBD
@RafaelGSS RafaelGSS mentioned this pull request May 10, 2022
RafaelGSS added a commit that referenced this pull request May 10, 2022
Notable changes:

* Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812

PR-URL: TBD
RafaelGSS added a commit that referenced this pull request May 10, 2022
Notable changes:

* Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 16, 2022
Notable changes:

* Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 16, 2022
Notable changes:

* This release updates OpenSSL to 3.0.3.
This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18.
See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines.

* *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022
* deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812
* deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022
* doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039
* doc: add release key for Juan Arboleda (Juan José) #42961
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812
* perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 16, 2022
OpenSSL

This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18.
See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines.

* deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022
* deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022

Other Notable changes

* *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812
* doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039
* doc: add release key for Juan Arboleda (Juan José) #42961
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812
* perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 16, 2022
OpenSSL

This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18.
See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines.

* deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022
* deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022

Other Notable Changes

* *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978
* deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059
* deps: upgrade npm to 8.9.0 (npm team) #42968
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812
* doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039
* doc: add release key for Juan Arboleda (Juan José) #42961
* (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768
* (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812
* perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 16, 2022
Notable changes:

OpenSSL 3.0.3

This update can be treated as a security release as the issues addressed
in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/
for more information on how the May 2022 OpenSSL releases affect other
Node.js release lines.

- deps: update archs files for quictls/openssl-3.0.3+quic
  (RafaelGSS) #43022
- deps: upgrade openssl sources to quictls/openssl-3.0.3
  (RafaelGSS) #43022

Other notable changes:

- _Revert_ "deps: add template for generated headers"
  (Daniel Bevenius) #42978
- deps: update undici to 5.2.0
  (Node.js GitHub Bot) #43059
- deps: upgrade npm to 8.9.0
  (npm team) #42968
- (SEMVER-MINOR) fs: add `read(buffer[, options])` versions
  (LiviaMedeiros) #42768
- (SEMVER-MINOR) http: added connection closing methods
  (Shogun) #42812
- doc: add LiviaMedeiros to collaborators
  (LiviaMedeiros) #43039
- doc: add release key for Juan Arboleda
  (Juan José) #42961
- (SEMVER-MINOR) fs: add `read(buffer[, options])` versions
  (LiviaMedeiros) #42768
- (SEMVER-MINOR) http: added connection closing methods
  (Paolo Insogna) #42812
- (SEMVER-MINOR) perf_hooks: add PerformanceResourceTiming
  (RafaelGSS) #42725

PR-URL: #43036
RafaelGSS added a commit that referenced this pull request May 17, 2022
Notable changes:

OpenSSL 3.0.3

This update can be treated as a security release as the issues addressed
in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/
for more information on how the May 2022 OpenSSL releases affect other
Node.js release lines.

- deps: update archs files for quictls/openssl-3.0.3+quic
  (RafaelGSS) #43022
- deps: upgrade openssl sources to quictls/openssl-3.0.3
  (RafaelGSS) #43022

Other notable changes:

- _Revert_ "deps: add template for generated headers"
  (Daniel Bevenius) #42978
- deps: update undici to 5.2.0
  (Node.js GitHub Bot) #43059
- deps: upgrade npm to 8.9.0
  (npm team) #42968
- (SEMVER-MINOR) fs: add `read(buffer[, options])` versions
  (LiviaMedeiros) #42768
- (SEMVER-MINOR) http: added connection closing methods
  (Shogun) #42812
- doc: add LiviaMedeiros to collaborators
  (LiviaMedeiros) #43039
- doc: add release key for Juan Arboleda
  (Juan José) #42961
- (SEMVER-MINOR) fs: add `read(buffer[, options])` versions
  (LiviaMedeiros) #42768
- (SEMVER-MINOR) http: added connection closing methods
  (Paolo Insogna) #42812
- (SEMVER-MINOR) perf_hooks: add PerformanceResourceTiming
  (RafaelGSS) #42725

PR-URL: #43036
@juanarbol
Copy link
Member

Some parts of this rely on top of #41263, I did not find a way to rapidly backport this

@ShogunPanda
Copy link
Contributor Author

@juanarbol Unfortunately I don't think it's possible without backporting both PR. The problem is that #41263 is semver-major

@juanarbol
Copy link
Member

That's a shame 💔

@SimenB
Copy link
Member

SimenB commented Nov 7, 2022

Sorry for posting on an old issue, but was #42812 (comment) ever done? Running git blame on the linked line of code seems to say "no" (unchanged for 14 months)

@ShogunPanda
Copy link
Contributor Author

@SimenB No, it was not done yet. I'll fix it soon.

jgirouard23

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to force close persistent connections
10 participants