Node.js V19 support (#4366)

.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:

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

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
     }

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'

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()
 })

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 })