feat: Use new Node.js' connection closing API if available #3925

ShogunPanda · 2022-05-23T11:00:56Z

This PR aims to leverage the new Node.js 18.2.0 APIs if available and changes the forceCloseConnection option as follows:

forceCloseConnection=false - No connection handling is done
forceCloseConnection=true - Use closeAllConnections if available, otherwise the fastify connection handling.
forceCloseConnection="idle" - Use closeIdleConnections if available, otherwise throws an error.

The option defaults to "idle" if the server supports closeIdleConnections, otherwise to false.

Fixes #3924.

mcollina

After seeing this... I would prefer a slightly different approach.

Add a closeConnectionsOnClose option at the factory or to the close() method. Three options: false, 'idle', 'all'.
The default value should 'idle' if it is available.
throw if closeConnectionOnExit is set to a value that is not available on the current Node.js runtime.

The new behavior is applied automatically to fastify.close() and no new methods are added.

ShogunPanda · 2022-05-23T11:08:34Z

Ok, makes sense. I'll rework it soon.

test/close.test.js

ShogunPanda · 2022-05-23T19:30:57Z

@mcollina As agreed, I've simplified this PR a lot.
No new API, the only two changes are the following:

When available by the serverFactory (which, for the default Node.js HTTP server means 18.2.0), if forceCloseConnections is true, then server.closeIdleConnections is used.
forceCloseConnections now defaults to true.

Hope this helps!

mcollina · 2022-05-23T20:15:43Z

Can you update the PR title?

jsumners · 2022-05-23T21:26:28Z

build/build-validation.js

@@ -24,7  24,7 @@ module.exports.defaultInitOptions = ${JSON.stringify(defaultInitOptions)}
 const defaultInitOptions = {
  connectionTimeout: 0, // 0 sec
  keepAliveTimeout: 72000, // 72 seconds
-  forceCloseConnections: false, // keep-alive connections
+  forceCloseConnections: true, // keep-alive connections


This is dangerous. There are ramifications that the user must know and account for. I disagree with enabling this by default.

I can easily revert this. But enabling this will IMHO make close behave a bit more like someone might expect.
Which ramification are you talking about? I can definitely update the docs if needed.

What ramifications? Those sockets are idle, there is no upstanding requests (minus the in-flight that we have not received yet).

On the other hand, making this the default improves the development experience and matches what people expects.

I vote for not changing the default.
Add new option for closing behavior and default as idle, and when forceCloseConnections is true. It will be equal to all (which means call closeAllConnections).

Note that forceCloseConnections is not equal to all right now, only idle.

@mcollina Actually, after checking the source code, I realized that fastify behavior is more similar to http.Server.closeAllConnections.
Despite of the internal variable naming of keepAliveConnections, the .close will destroy ALL connected clients, not idle one only.
Shall I changed the method used?

Yes, the current implementation is actually .closeAllConnections. It will destory all connections no matter the state.

The value from forceCloseConnections should be translated as follow to match the current behavior.

true should translate to all

false should translate to idle or false. I prefer idle here because it do not harms anyone if it close the idle connections.

Adding an extra option to explicitly disable the auto closing connections feature (in separate PR).

I just updated the PR. I think that false should disable connection closing at all. The next PR might instead add idle for the other behavior.

Reason is fastify is not capable at the moment of closing idle connections only.

If in this pr, forceCloseConnections is equal to all, I think it's better to use false by default

fastify.js

ShogunPanda · 2022-05-24T09:18:38Z

Can you update the PR title?

I've update it and the description as well.

fastify.js

lib/route.js

mcollina · 2022-05-24T15:26:49Z

fastify.js

-          conn.destroy()
-          fastify[kKeepAliveConnections].delete(conn)
+        if (serverSupportsCloseAllConnections && options.forceCloseConnections) {
+          instance.server.closeAllConnections()


Suggested change

instance.server.closeAllConnections()

instance.server.closeIdleConnections()

Well, here's the thing. If we just want to translate the existing behavior to the Node.js core provided one, then it's equivalent to closeAllConnections.

If instead we want to change the semantics, then yes, I will apply your suggestion.

ah gosh. Let's not flip the default and add an option to close only the idle connections.

lib/route.js

mcollina · 2022-05-27T14:00:16Z

Can you rebase and regenerate the configValidator?

ShogunPanda · 2022-05-27T14:23:51Z

@mcollina Done

ShogunPanda · 2022-05-27T14:24:12Z

(sorry, my finger slipped)

mcollina

What I would like to achieve is that by default, if closeIdleConnections is available, it's used to shorten down the wait time.

mcollina · 2022-05-31T08:17:50Z

build/build-validation.js

@@ -24,7  24,7 @@ module.exports.defaultInitOptions = ${JSON.stringify(defaultInitOptions)}
 const defaultInitOptions = {
  connectionTimeout: 0, // 0 sec
  keepAliveTimeout: 72000, // 72 seconds
-  forceCloseConnections: false, // keep-alive connections
+  forceCloseConnections: true, // keep-alive connections


ShogunPanda · 2022-05-31T14:48:14Z

I have updated the PR completely, see the new description above. How does it look now?

@mcollina I haven't changed the default as new APIs are not available in Node 16. We might flip this later once 18 reaches LTS.

docs/Reference/Server.md

mcollina

lgtm

lib/warnings.js

fastify.js

test/close.test.js

ShogunPanda · 2022-06-03T13:28:52Z

I've force pushed the branch again. PTAL!

mcollina

lgtm

github-actions · 2023-06-05T00:23:45Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mcollina requested changes May 23, 2022

View reviewed changes

metcoder95 reviewed May 23, 2022

View reviewed changes

test/close.test.js Outdated Show resolved Hide resolved

mcollina requested a review from jsumners May 23, 2022 20:15

jsumners reviewed May 23, 2022

View reviewed changes

climba03003 reviewed May 24, 2022

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

ShogunPanda changed the title ~~feat: Added server.closeAllConnections and server.closeIdleConnections.~~ feat: Use http.Server.closeIdleConnections if available May 24, 2022

xtx1130 reviewed May 24, 2022

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

jsumners reviewed May 25, 2022

View reviewed changes

lib/route.js Outdated Show resolved Hide resolved

mcollina reviewed May 25, 2022

View reviewed changes

jsumners reviewed May 25, 2022

View reviewed changes

lib/route.js Outdated Show resolved Hide resolved

mcollina added this to the v4.0.0 milestone May 26, 2022

ShogunPanda closed this May 27, 2022

ShogunPanda reopened this May 27, 2022

jsumners added the notable label May 27, 2022

mcollina reviewed May 31, 2022

View reviewed changes

ShogunPanda changed the title ~~feat: Use http.Server.closeIdleConnections if available~~ feat: Use http.Server.closeIdleConnections and http.Server.closeAllConnections if available May 31, 2022

jsumners reviewed May 31, 2022

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

mcollina approved these changes May 31, 2022

View reviewed changes

climba03003 reviewed Jun 1, 2022

View reviewed changes

lib/warnings.js Outdated Show resolved Hide resolved

fastify.js Outdated Show resolved Hide resolved

mcollina reviewed Jun 1, 2022

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

test/close.test.js Outdated Show resolved Hide resolved

ShogunPanda changed the title ~~feat: Use http.Server.closeIdleConnections and http.Server.closeAllConnections if available~~ feat: Use new Node.js' connection closing API if available Jun 3, 2022

feat: Use new Node.js' connection closing API if available

cb40581

mcollina approved these changes Jun 3, 2022

View reviewed changes

jsumners approved these changes Jun 3, 2022

View reviewed changes

kibertoad merged commit f3139a0 into fastify:main Jun 3, 2022

ShogunPanda deleted the close-all-connections branch June 4, 2022 06:35

github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2023

jsumners added notable-change A change that should be explicitly outlined in release notes and migration guides and removed notable labels Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use new Node.js' connection closing API if available #3925

feat: Use new Node.js' connection closing API if available #3925

ShogunPanda commented May 23, 2022 •

edited

Loading

mcollina left a comment

ShogunPanda commented May 23, 2022

ShogunPanda commented May 23, 2022

mcollina commented May 23, 2022

jsumners May 23, 2022

ShogunPanda May 24, 2022

mcollina May 24, 2022

climba03003 May 24, 2022 •

edited

Loading

mcollina May 24, 2022

ShogunPanda May 24, 2022

climba03003 May 24, 2022 •

edited

Loading

ShogunPanda May 24, 2022

xtx1130 May 24, 2022

mcollina May 31, 2022

ShogunPanda commented May 24, 2022

mcollina May 24, 2022

ShogunPanda May 27, 2022

mcollina May 27, 2022

mcollina commented May 27, 2022

ShogunPanda commented May 27, 2022

ShogunPanda commented May 27, 2022

mcollina left a comment

mcollina May 31, 2022

ShogunPanda commented May 31, 2022

mcollina left a comment

ShogunPanda commented Jun 3, 2022

mcollina left a comment

github-actions bot commented Jun 5, 2023

	instance.server.closeAllConnections()
	instance.server.closeIdleConnections()

feat: Use new Node.js' connection closing API if available #3925

feat: Use new Node.js' connection closing API if available #3925

Conversation

ShogunPanda commented May 23, 2022 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

ShogunPanda commented May 23, 2022

ShogunPanda commented May 23, 2022

mcollina commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

climba03003 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

climba03003 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunPanda commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented May 27, 2022

ShogunPanda commented May 27, 2022

ShogunPanda commented May 27, 2022

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunPanda commented May 31, 2022

mcollina left a comment

Choose a reason for hiding this comment

ShogunPanda commented Jun 3, 2022

mcollina left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2023

ShogunPanda commented May 23, 2022 •

edited

Loading

climba03003 May 24, 2022 •

edited

Loading

climba03003 May 24, 2022 •

edited

Loading