From 268ee8bbf8a905e24b5d0a0350554456fa594919 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 20 Oct 2022 18:15:26 +0200 Subject: [PATCH 1/7] Fixed test/close.test.js for Node v19 Signed-off-by: Matteo Collina --- test/close.test.js | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/test/close.test.js b/test/close.test.js index 90dd53934b..ac69d43278 100644 --- a/test/close.test.js +++ b/test/close.test.js @@ -6,6 +6,7 @@ const t = require('tap') const test = t.test const Fastify = require('..') const { Client } = require('undici') +const semver = require('semver') test('close callback', t => { t.plan(4) @@ -202,7 +203,8 @@ test('Should return error while closing (callback) - injection', t => { }) }) -t.test('Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false', t => { +const isV19plus = semver.satisfies(process.version, '>= v19.0.0') +t.test('Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false', { skip: isV19plus }, t => { const fastify = Fastify({ return503OnClosing: false, forceCloseConnections: false @@ -240,6 +242,39 @@ t.test('Current opened connection should continue to work after closing and retu }) }) +t.test('Current opened connection should NOT continue to work after closing and return "connection: close" header - return503OnClosing: false', { skip: !isV19plus }, t => { + t.plan(4) + const fastify = Fastify({ + return503OnClosing: false, + forceCloseConnections: false + }) + + fastify.get('/', (req, reply) => { + fastify.close() + reply.send({ hello: 'world' }) + }) + + fastify.listen({ port: 0 }, err => { + t.error(err) + + const port = fastify.server.address().port + const client = net.createConnection({ port }, () => { + client.write('GET / HTTP/1.1\r\n\r\n') + + client.on('error', function (err) { + t.ok(err) + }) + + client.once('data', data => { + t.match(data.toString(), /Connection:\s*keep-alive/i) + t.match(data.toString(), /200 OK/i) + + client.write('GET / HTTP/1.1\r\n\r\n') + }) + }) + }) +}) + t.test('Current opened connection should not accept new incoming connections', t => { t.plan(3) const fastify = Fastify({ forceCloseConnections: false }) From 17a1aa89ddb211276717e8194f41e3f32f5eb2c2 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 10:59:32 +0200 Subject: [PATCH 2/7] fix close tests Signed-off-by: Matteo Collina --- fastify.js | 3 +- lib/route.js | 6 +- test/close-pipelining.test.js | 109 +++++++++++++++++++++------------- test/close.test.js | 4 +- 4 files changed, 76 insertions(+), 46 deletions(-) diff --git a/fastify.js b/fastify.js index ac8671db32..c50ddf2c9e 100644 --- a/fastify.js +++ b/fastify.js @@ -410,11 +410,12 @@ function fastify (options) { /* istanbul ignore next: Cannot test this without Node.js core support */ if (forceCloseConnections === 'idle') { + // Not needed in Node 19 instance.server.closeIdleConnections() /* istanbul ignore next: Cannot test this without Node.js core support */ } else if (serverHasCloseAllConnections && forceCloseConnections) { instance.server.closeAllConnections() - } else { + } else if (forceCloseConnections === true) { for (const conn of fastify[kKeepAliveConnections]) { // We must invoke the destroy method instead of merely unreffing // the sockets. If we only unref, then the callback passed to diff --git a/lib/route.js b/lib/route.js index 286378604b..172d51edfa 100644 --- a/lib/route.js +++ b/lib/route.js @@ -398,11 +398,15 @@ function buildRouting (options) { if (closing === true) { /* istanbul ignore next mac, windows */ if (req.httpVersionMajor !== 2) { - res.once('finish', () => req.destroy()) + // res.once('finish', () => req.destroy()) res.setHeader('Connection', 'close') } + /* istanbul ignore else */ if (return503OnClosing) { + // On Node v19 we cannot test this behavior as it won't be necessary + // anymore. It will close all the idle connections before they reach this + // stage. const headers = { 'Content-Type': 'application/json', 'Content-Length': '80' diff --git a/test/close-pipelining.test.js b/test/close-pipelining.test.js index 1733674a6b..3f4b273b83 100644 --- a/test/close-pipelining.test.js +++ b/test/close-pipelining.test.js @@ -4,42 +4,72 @@ const t = require('tap') const test = t.test const Fastify = require('..') const { Client } = require('undici') +const semver = require('semver') -test('Should return 503 while closing - pipelining', t => { +test('Should return 503 while closing - pipelining', async t => { const fastify = Fastify({ return503OnClosing: true, forceCloseConnections: false }) + fastify.get('/', async (req, reply) => { + fastify.close() + return { hello: 'world' } + }) + + await fastify.listen({ port: 0 }) + + const instance = new Client('http://localhost:' + fastify.server.address().port, { + pipelining: 2 + }) + + const codes = [200, 200, 503] + const responses = await Promise.all([ + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }) + ]) + const actual = responses.map(r => r.statusCode) + + t.same(actual, codes) + + await instance.close() +}) + +const isV19plus = semver.satisfies(process.version, '>= v19.0.0') +test('Should not return 503 while closing - pipelining - return503OnClosing: false, skip Node >= v19.x', { skip: isV19plus }, async t => { + const fastify = Fastify({ + return503OnClosing: false, + forceCloseConnections: false + }) + fastify.get('/', (req, reply) => { fastify.close() reply.send({ hello: 'world' }) }) - fastify.listen({ port: 0 }, async err => { - t.error(err) - - const instance = new Client('http://localhost:' + fastify.server.address().port, { - pipelining: 1 - }) - - const codes = [200, 503] - for (const code of codes) { - instance.request( - { path: '/', method: 'GET' } - ).then(data => { - t.equal(data.statusCode, code) - }).catch((e) => { - t.fail(e) - }) - } - instance.close(() => { - t.end('Done') - }) + await fastify.listen({ port: 0 }) + + const instance = new Client('http://localhost:' + fastify.server.address().port, { + pipelining: 2 }) + + const codes = [200, 200, 200] + const responses = await Promise.all([ + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }) + ]) + const actual = responses.map(r => r.statusCode) + + t.same(actual, codes) + + await instance.close() }) -test('Should not return 503 while closing - pipelining - return503OnClosing', t => { +test('Should close the socket abruptly - pipelining - return503OnClosing: false, skip Node < v19.x', { skip: !isV19plus }, async t => { + // Since Node v19, we will always invoke server.closeIdleConnections() + // therefore our socket will be closed const fastify = Fastify({ return503OnClosing: false, forceCloseConnections: false @@ -50,25 +80,20 @@ test('Should not return 503 while closing - pipelining - return503OnClosing', t reply.send({ hello: 'world' }) }) - fastify.listen({ port: 0 }, err => { - t.error(err) - - const instance = new Client('http://localhost:' + fastify.server.address().port, { - pipelining: 1 - }) - - const codes = [200, 200] - for (const code of codes) { - instance.request( - { path: '/', method: 'GET' } - ).then(data => { - t.equal(data.statusCode, code) - }).catch((e) => { - t.fail(e) - }) - } - instance.close(() => { - t.end('Done') - }) + await fastify.listen({ port: 0 }) + + const instance = new Client('http://localhost:' + fastify.server.address().port, { + pipelining: 2 }) + + const responses = await Promise.allSettled([ + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }), + instance.request({ path: '/', method: 'GET' }) + ]) + t.equal(responses[0].status, 'fulfilled') + t.equal(responses[1].status, 'rejected') + t.equal(responses[2].status, 'rejected') + + await instance.close() }) diff --git a/test/close.test.js b/test/close.test.js index ac69d43278..b2e7626949 100644 --- a/test/close.test.js +++ b/test/close.test.js @@ -204,7 +204,7 @@ test('Should return error while closing (callback) - injection', t => { }) const isV19plus = semver.satisfies(process.version, '>= v19.0.0') -t.test('Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false', { skip: isV19plus }, t => { +t.test('Current opened connection should continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node >= v19.x', { skip: isV19plus }, t => { const fastify = Fastify({ return503OnClosing: false, forceCloseConnections: false @@ -242,7 +242,7 @@ t.test('Current opened connection should continue to work after closing and retu }) }) -t.test('Current opened connection should NOT continue to work after closing and return "connection: close" header - return503OnClosing: false', { skip: !isV19plus }, t => { +t.test('Current opened connection should NOT continue to work after closing and return "connection: close" header - return503OnClosing: false, skip Node < v19.x', { skip: !isV19plus }, t => { t.plan(4) const fastify = Fastify({ return503OnClosing: false, From 37e6ea1cb44c31b5b52c6cca4fea7a60200e0e97 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 11:11:47 +0200 Subject: [PATCH 3/7] fix broken tests Signed-off-by: Matteo Collina --- lib/contentTypeParser.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/contentTypeParser.js b/lib/contentTypeParser.js index a1eebeb3a0..8f46dd0b61 100644 --- a/lib/contentTypeParser.js +++ b/lib/contentTypeParser.js @@ -190,6 +190,9 @@ function rawBody (request, reply, options, parser, done) { : Number(request.headers['content-length']) if (contentLength > limit) { + // We must close the connection as the client is going + // to send this data anyway + reply.header('connection', 'close') reply.send(new FST_ERR_CTP_BODY_TOO_LARGE()) return } @@ -243,6 +246,7 @@ function rawBody (request, reply, options, parser, done) { } if (!Number.isNaN(contentLength) && (payload.receivedEncodedLength || receivedLength) !== contentLength) { + reply.header('connection', 'close') reply.send(new FST_ERR_CTP_INVALID_CONTENT_LENGTH()) return } From 3469ef768dfb1122aadee5eb55b2b011af8c37f8 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 11:39:50 +0200 Subject: [PATCH 4/7] add node v19 to CI Signed-off-by: Matteo Collina --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a6baf63bc..24cc39ad70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,7 +72,7 @@ jobs: strategy: matrix: - node-version: [14, 16, 18] + node-version: [14, 16, 18, 19] os: [macos-latest, ubuntu-latest, windows-latest] steps: From 2d4b5f956c286b0c59abc5052dd55e4d1df9d9ad Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 11:41:38 +0200 Subject: [PATCH 5/7] fixup Signed-off-by: Matteo Collina --- lib/route.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/route.js b/lib/route.js index 172d51edfa..5e7aed9890 100644 --- a/lib/route.js +++ b/lib/route.js @@ -398,7 +398,6 @@ function buildRouting (options) { if (closing === true) { /* istanbul ignore next mac, windows */ if (req.httpVersionMajor !== 2) { - // res.once('finish', () => req.destroy()) res.setHeader('Connection', 'close') } From 89e64e7546d80e0606bf5837993310fe75e96d3e Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 11:58:26 +0200 Subject: [PATCH 6/7] fixup Signed-off-by: Matteo Collina --- test/close.test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/close.test.js b/test/close.test.js index b2e7626949..b0c78877ff 100644 --- a/test/close.test.js +++ b/test/close.test.js @@ -261,8 +261,14 @@ t.test('Current opened connection should NOT continue to work after closing and const client = net.createConnection({ port }, () => { client.write('GET / HTTP/1.1\r\n\r\n') - client.on('error', function (err) { - t.ok(err) + client.on('error', function () { + // Dependending on the Operating System + // the socket could error or not. + // However, it will always be closed. + }) + + client.on('close', function () { + t.pass('close') }) client.once('data', data => { From 7747088d659df60dc4984e934ab9e2b2f1775fd3 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 21 Oct 2022 12:07:29 +0200 Subject: [PATCH 7/7] fixup Signed-off-by: Matteo Collina --- lib/route.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/route.js b/lib/route.js index 5e7aed9890..fc4526de0f 100644 --- a/lib/route.js +++ b/lib/route.js @@ -401,6 +401,7 @@ function buildRouting (options) { res.setHeader('Connection', 'close') } + // TODO remove return503OnClosing after Node v18 goes EOL /* istanbul ignore else */ if (return503OnClosing) { // On Node v19 we cannot test this behavior as it won't be necessary