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: 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/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 } diff --git a/lib/route.js b/lib/route.js index 286378604b..fc4526de0f 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.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 + // 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 90dd53934b..b0c78877ff 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 Node >= v19.x', { skip: isV19plus }, t => { const fastify = Fastify({ return503OnClosing: false, forceCloseConnections: false @@ -240,6 +242,45 @@ 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 Node < v19.x', { 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 () { + // 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 => { + 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 })