Add keep-alive connection tracking and reaping (resolve #3617) #3619

jsumners · 2022-01-14T22:09:54Z

2022-01-14:

This is a proof of concept. The code is bare minimum to add the feature set. It needs to be cleaned up to follow proper internals code if the feature set will be accepted. Also, automated tests will be needed. The PR is created to get feedback on the implementation and discussion on whether to move forward.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

fastify.js

lib/route.js

mcollina · 2022-01-15T20:04:25Z

The predominant thing you are testing with load testing new connection for every request is Node.js ability to accept them, the TCP settings of your machine and similar things. They are not useful for evaluating the performance of a framework.

Note that for this specific case I expect the actual performance to be worse for short lived connections that do 2-3 requests and then are closed (we can benchmark this with autocannon) as they will enter/exit the Set often.

A solution for this is to put it behind a flag.

jsumners · 2022-01-15T20:29:40Z

Yes. The full implementation would be behind a flag. Doing this automatically would be a breaking change and would not be something everyone wants without explicitly saying so.

mcollina · 2022-01-15T21:21:13Z

Then I'm good with this approach.

jsumners · 2022-01-17T13:39:48Z

I have tested two methods of skipping connection tracking by default. First, by providing a "Set" that does nothing. Second by passing through a boolean that skips the whole "add to set" path in the route handler. I am seeing no difference between the two methods. Therefore, I am going to opt for the easier to maintain "noop Set" implementation.

Benchmark for noop Set

Node: 12
PR: [1] 1756k requests in 30.12s, 328 MB read
MAIN: [1] 1724k requests in 30.13s, 322 MB read

Node: 14
PR: [1] 1490k requests in 30.06s, 278 MB read
MAIN: [1] 1443k requests in 30.07s, 270 MB read

Node: 16
PR: [1] 1032k requests in 30.17s, 193 MB read
MAIN: [1] 1006k requests in 30.08s, 188 MB read

Benchmark for boolean toggle

Node: 12
PR: [1] 1677k requests in 30.14s, 313 MB read
MAIN: [1] 1683k requests in 30.16s, 315 MB read

Node: 14
PR: [1] 1832k requests in 30.11s, 342 MB read
MAIN: [1] 1917k requests in 30.05s, 358 MB read

Node: 16
PR: [1] 1634k requests in 30.11s, 305 MB read
MAIN: [1] 1642k requests in 30.08s, 307 MB read

mcollina · 2022-01-17T13:50:13Z

The noop set would be ok for me.

test/close.test.js

test/noop-set.test.js

jsumners · 2022-01-17T18:47:09Z

I'm getting the exact same missing coverage on the main branch.

Eomm · 2022-01-17T18:48:25Z

test/close.test.js

+        // for the client to have seen the socket get destroyed. The mere fact
+        // that we have reached this callback is enough indication that the
+        // feature being tested works as designed.
+        t.equal(client.closed, false)


Should the client receive an error event due to the socket being closed?

Eomm · 2022-01-17T20:12:18Z

@jsumners I remembered this pr
#3456

The issue is the localhost string

github-actions · 2022-01-18T13:56:16Z

Node: 12
PR: [1] 1990k requests in 30.16s, 372 MB read
MAIN: [1] 1905k requests in 30.1s, 356 MB read

Node: 14
PR: [1] 1464k requests in 30.07s, 274 MB read
MAIN: [1] 1455k requests in 30.06s, 272 MB read

Node: 16
PR: [1] 1547k requests in 30.19s, 289 MB read
MAIN: [1] 1784k requests in 30.11s, 333 MB read

jsumners · 2022-01-18T13:59:15Z

package.json

@@ -16,7 +16,7 @@
    "lint:typescript": "eslint -c types/.eslintrc.json types/**/*.d.ts test/types/**/*.test-d.ts",
    "prepublishOnly": "tap --no-check-coverage test/internals/version.test.js",
    "test": "npm run lint && npm run unit && npm run test:typescript",
-    "test:ci": "npm run unit -- --cov --coverage-report=lcovonly && npm run test:typescript",
+    "test:ci": "npm run unit -- -R terse --cov --coverage-report=lcovonly && npm run test:typescript",


I'll call this out before anyone else:

I added this because one of the macOS runs flaked. We have so many tests that scrolling through them to figure out why that run flaked was effectively impossible. By using -R terse, only the failures will be published to the test run logs. This will make it far easier to find the problems.

THANKS!!! I didn't know this was possiblez

scmorse · 2022-01-18T14:34:41Z

lib/noop-set.js

+    [Symbol.iterator]: function * () {},
+    add () {},
+    delete () {},
+    has () { return true }


Wouldn't the noop set be empty, and return false for all .has() calls?

Returning true prevents new connections from being added to the set and thus avoids a branch and extra function invocation (.add()).

scmorse · 2022-01-18T14:42:12Z

docs/Reference/Server.md

+persistent connections to timeout first. Important: connections are not
+inspected to determine if requests have been completed.


Do you think it could be possible to order the shutdown cycle in this way:

Disable acceptance of new requests

In-progress request handlers are allowed to complete

Once all handlers have completed, close the sockets with keep-alive connections?

As far as I know, it is impossible to determine if a request is pending via the socket. We only retain the sockets so that we do not keep the full request(s) in memory for the duration of the persistent connection.

Node.js knows btw, however that info is private

As far as I know, it is impossible to determine if a request is pending via the socket.

Wouldn't we know via fastify tracking handler()s that are pending, rather than the socket?

We only retain the sockets so that we do not keep the full request(s) in memory for the duration of the persistent connection.

Yeah, I'm talking about while the request is still active and the connection is active.

@scmorse I believe this PR as written does the best we can do in Fastify without significant impact to performance. Any other changes will rely on nodejs/node#41578 going through.

mcollina

lgtm

We might want to add something similar for HTTP/2, but I would push that to a separate issue.

mattbishop · 2022-01-27T14:47:58Z

Folks, fastify.d.ts was not updated to include this new server option. I can file a ticket if necessary.

jsumners · 2022-01-27T14:50:03Z

That is not a requirement for Fastify PRs. Not everyone knows TypeScript. A subsequent PR has already been created #3646.

jsumners requested a review from mcollina January 14, 2022 22:10

jsumners added the benchmark Label to run benchmark against PR and main branch label Jan 14, 2022

This comment has been minimized.

Sign in to view

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jan 14, 2022

Eomm reviewed Jan 15, 2022

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

lib/route.js Outdated Show resolved Hide resolved

jsumners added the benchmark Label to run benchmark against PR and main branch label Jan 15, 2022

This comment has been minimized.

Sign in to view

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jan 15, 2022

mcollina reviewed Jan 15, 2022

View reviewed changes

lib/route.js Outdated Show resolved Hide resolved

lib/route.js Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jsumners added the benchmark Label to run benchmark against PR and main branch label Jan 17, 2022

This comment has been minimized.

Sign in to view

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jan 17, 2022

jsumners added the benchmark Label to run benchmark against PR and main branch label Jan 17, 2022

This comment has been minimized.

Sign in to view

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jan 17, 2022

jsumners force-pushed the issue-3617 branch from e9e4bf6 to 5725170 Compare January 17, 2022 14:07

jsumners commented Jan 17, 2022

View reviewed changes

test/close.test.js Show resolved Hide resolved

mcollina reviewed Jan 17, 2022

View reviewed changes

test/noop-set.test.js Show resolved Hide resolved

mcollina reviewed Jan 17, 2022

View reviewed changes

test/noop-set.test.js Show resolved Hide resolved

jsumners force-pushed the issue-3617 branch 2 times, most recently from a1c2b23 to d6bf012 Compare January 17, 2022 18:36

Eomm reviewed Jan 17, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

jsumners mentioned this pull request Jan 17, 2022

Coverage not 100% due to SSL cert check issue #3634

Closed

2 tasks

This comment has been minimized.

Sign in to view

Add keep-alive connection tracking and reaping

bb8f285

jsumners force-pushed the issue-3617 branch from 861c5a5 to bb8f285 Compare January 18, 2022 13:51

jsumners marked this pull request as ready for review January 18, 2022 13:51

jsumners requested review from mcollina and Eomm January 18, 2022 13:52

jsumners added the benchmark Label to run benchmark against PR and main branch label Jan 18, 2022

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Jan 18, 2022

jsumners commented Jan 18, 2022

View reviewed changes

scmorse reviewed Jan 18, 2022

View reviewed changes

jsumners mentioned this pull request Jan 18, 2022

Add ability to force close persistent connections nodejs/node#41578

Closed

mcollina approved these changes Jan 18, 2022

View reviewed changes

jsumners merged commit 2c1505a into fastify:main Jan 18, 2022

jsumners deleted the issue-3617 branch January 18, 2022 15:45

jsumners mentioned this pull request Jan 18, 2022

Draining connections on close #3617

Closed

fastify locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keep-alive connection tracking and reaping (resolve #3617) #3619

Add keep-alive connection tracking and reaping (resolve #3617) #3619

jsumners commented Jan 14, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mcollina commented Jan 15, 2022

jsumners commented Jan 15, 2022

mcollina commented Jan 15, 2022

This comment has been minimized.

This comment has been minimized.

jsumners commented Jan 17, 2022

mcollina commented Jan 17, 2022

jsumners commented Jan 17, 2022

Eomm Jan 17, 2022

jsumners Jan 17, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Eomm commented Jan 17, 2022

github-actions bot commented Jan 18, 2022

jsumners Jan 18, 2022

mcollina Jan 18, 2022

scmorse Jan 18, 2022

jsumners Jan 18, 2022

scmorse Jan 18, 2022

jsumners Jan 18, 2022

mcollina Jan 18, 2022

scmorse Jan 18, 2022

jsumners Jan 18, 2022

mcollina left a comment

mattbishop commented Jan 27, 2022

jsumners commented Jan 27, 2022

		persistent connections to timeout first. Important: connections are not
		inspected to determine if requests have been completed.

Add keep-alive connection tracking and reaping (resolve #3617) #3619

Add keep-alive connection tracking and reaping (resolve #3617) #3619

Conversation

jsumners commented Jan 14, 2022

Checklist

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mcollina commented Jan 15, 2022

jsumners commented Jan 15, 2022

mcollina commented Jan 15, 2022

This comment has been minimized.

This comment has been minimized.

jsumners commented Jan 17, 2022

mcollina commented Jan 17, 2022

jsumners commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Eomm commented Jan 17, 2022

github-actions bot commented Jan 18, 2022

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

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

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mattbishop commented Jan 27, 2022

jsumners commented Jan 27, 2022