-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add keep-alive connection tracking and reaping (resolve #3617) #3619
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
'use strict' | ||
|
||
module.exports = function noopSet () { | ||
return { | ||
[Symbol.iterator]: function * () {}, | ||
add () {}, | ||
delete () {}, | ||
has () { return true } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't the noop set be empty, and return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. THANKS!!! I didn't know this was possiblez |
||
"test:report": "npm run lint && npm run unit:report && npm run test:typescript", | ||
"test:typescript": "tsd", | ||
"unit": "tap -J test/*.test.js test/*/*.test.js", | ||
|
@@ -129,6 +129,7 @@ | |
"@types/pino": "^6.0.1", | ||
"@typescript-eslint/eslint-plugin": "^4.5.0", | ||
"@typescript-eslint/parser": "^4.5.0", | ||
"JSONStream": "^1.3.5", | ||
"ajv": "^6.0.0", | ||
"ajv-errors": "^1.0.1", | ||
"ajv-formats": "^2.1.1", | ||
|
@@ -157,7 +158,6 @@ | |
"hsts": "^2.2.0", | ||
"http-errors": "^2.0.0", | ||
"ienoopen": "^1.1.0", | ||
"JSONStream": "^1.3.5", | ||
"license-checker": "^25.0.1", | ||
"pem": "^1.14.4", | ||
"proxyquire": "^2.1.3", | ||
|
@@ -173,7 +173,7 @@ | |
"then-sleep": "^1.0.1", | ||
"tsd": "^0.19.0", | ||
"typescript": "^4.0.2", | ||
"undici": "^3.3.5", | ||
"undici": "^3.3.6", | ||
"x-xss-protection": "^2.0.0", | ||
"yup": "^0.32.0" | ||
}, | ||
|
@@ -206,4 +206,4 @@ | |
"tsd": { | ||
"directory": "test/types" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,3 +291,41 @@ test('Cannot be reopened the closed server has listen callback', async t => { | |
t.ok(err) | ||
}) | ||
}) | ||
|
||
test('shutsdown while keep-alive connections are active (non-async)', t => { | ||
jsumners marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.plan(5) | ||
|
||
const timeoutTime = 2 * 60 * 1000 | ||
const fastify = Fastify({ forceCloseConnections: true }) | ||
|
||
fastify.server.setTimeout(timeoutTime) | ||
fastify.server.keepAliveTimeout = timeoutTime | ||
|
||
fastify.get('/', (req, reply) => { | ||
reply.send({ hello: 'world' }) | ||
}) | ||
|
||
fastify.listen(0, (err, address) => { | ||
t.error(err) | ||
|
||
const client = new Client( | ||
'http://localhost:' + fastify.server.address().port, | ||
{ keepAliveTimeout: 1 * 60 * 1000 } | ||
) | ||
client.request({ path: '/', method: 'GET' }, (err, response) => { | ||
t.error(err) | ||
t.equal(client.closed, false) | ||
|
||
fastify.close((err) => { | ||
t.error(err) | ||
|
||
// Due to the nature of the way we reap these keep-alive connections, | ||
// there hasn't been enough time before the server fully closed in order | ||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the client receive an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe? |
||
}) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
'use strict' | ||
|
||
const tap = require('tap') | ||
const noopSet = require('../lib/noop-set') | ||
|
||
tap.test('does a lot of nothing', async t => { | ||
const aSet = noopSet() | ||
t.type(aSet, 'object') | ||
|
||
const item = {} | ||
aSet.add(item) | ||
jsumners marked this conversation as resolved.
Show resolved
Hide resolved
|
||
aSet.add({ another: 'item' }) | ||
jsumners marked this conversation as resolved.
Show resolved
Hide resolved
|
||
aSet.delete(item) | ||
t.equal(aSet.has(item), true) | ||
|
||
for (const i of aSet) { | ||
t.fail('should not have any items', i) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it could be possible to order the shutdown cycle in this way:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node.js knows btw, however that info is private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we know via fastify tracking
handler()
s that are pending, rather than the socket?Yeah, I'm talking about while the request is still active and the connection is active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.