fix(#5180): close secondary bindings after primary is closed (#5201)

* fix(#5180): close secondary bindings after primary is closed

* test: handle ipv6 correctly

lib/server.js

@@ -163,29 +163,6 @@ function multipleBindings (mainServer, httpHandler, serverOpts, listenOptions, o
               /* istanbul ignore next: the else won't be taken unless listening fails */
               if (!_ignoreErr) {
                 this[kServerBindings].push(secondaryServer)
-                // Due to the nature of the feature is not possible to know
-                // ahead of time the number of bindings that will be made.
-                // For instance, each binding is hooked to be closed at their own
-                // pace through the `onClose` hook.
-                // It also allows them to handle possible connections already
-                // attached to them if any.
-                /* c8 ignore next 20 */
-                this.onClose((instance, done) => {
-                  if (instance[kState].listening) {
-                    // No new TCP connections are accepted
-                    // We swallow any error from the secondary
-                    // server
-                    secondaryServer.close(() => done())
-                    if (serverOpts.forceCloseConnections === 'idle') {
-                      // Not needed in Node 19
-                      secondaryServer.closeIdleConnections()
-                    } else if (typeof secondaryServer.closeAllConnections === 'function' && serverOpts.forceCloseConnections) {
-                      secondaryServer.closeAllConnections()
-                    }
-                  } else {
-                    done()
-                  }
-                })
               }
 
               if (bound === binding) {
@@ -196,7 +173,23 @@ function multipleBindings (mainServer, httpHandler, serverOpts, listenOptions, o
           })
 
           const secondaryServer = getServerInstance(serverOpts, httpHandler)
-          const closeSecondary = () => { secondaryServer.close(() => { }) }
+          const closeSecondary = () => {
+            // To avoid fall into situations where the close of the
+            // secondary server is triggered before the preClose hook
+            // is done running, we better wait until the main server
+            // is closed.
+            // No new TCP connections are accepted
+            // We swallow any error from the secondary
+            // server
+            secondaryServer.close(() => {})
+            if (serverOpts.forceCloseConnections === 'idle') {
+              // Not needed in Node 19
+              secondaryServer.closeIdleConnections()
+            } else if (typeof secondaryServer.closeAllConnections === 'function' && serverOpts.forceCloseConnections) {
+              secondaryServer.closeAllConnections()
+            }
+          }
+
           secondaryServer.on('upgrade', mainServer.emit.bind(mainServer, 'upgrade'))
           mainServer.on('unref', closeSecondary)
           mainServer.on('close', closeSecondary)

test/server.test.js

@@ -4,6 +4,7 @@ const t = require('tap')
 const test = t.test
 const Fastify = require('..')
 const semver = require('semver')
+const undici = require('undici')
 
 test('listen should accept null port', t => {
   t.plan(1)
@@ -123,3 +124,53 @@ test('abort signal', { skip: semver.lt(process.version, '16.0.0') }, t => {
 
   t.end()
 })
+
+t.test('#5180 - preClose should be called before closing secondary server', t => {
+  t.plan(2)
+  const fastify = Fastify({ forceCloseConnections: true })
+  let flag = false
+  t.teardown(fastify.close.bind(fastify))
+
+  fastify.addHook('preClose', async () => {
+    flag = true
+  })
+
+  fastify.get('/', async (req, reply) => {
+    await new Promise((resolve) => {
+      setTimeout(() => resolve(1), 1000)
+    })
+
+    return { hello: 'world' }
+  })
+
+  fastify.listen({ port: 0 }, (err) => {
+    t.error(err)
+    const addresses = fastify.addresses()
+    const mainServerAddress = fastify.server.address()
+    let secondaryAddress
+    for (const addr of addresses) {
+      if (addr.family !== mainServerAddress.family) {
+        secondaryAddress = addr
+        secondaryAddress.address = secondaryAddress.family === 'IPv6'
+          ? `[${secondaryAddress.address}]`
+          : secondaryAddress.address
+        break
+      }
+    }
+
+    if (!secondaryAddress) {
+      t.pass('no secondary server')
+      return
+    }
+
+    undici.request(`http://${secondaryAddress.address}:${secondaryAddress.port}/`)
+      .then(
+        () => { t.fail('Request should not succeed') },
+        () => { t.ok(flag) }
+      )
+
+    setTimeout(() => {
+      fastify.close()
+    }, 250)
+  })
+})