graceful shutdown (#60059)

- Both the standalone server and the `startServer` function it calls attempt to stop the server on `SIGINT` and `SIGTERM` in different ways. This lets `server.js` yield to `startServer` - The cleanup function in `startServer` was not waiting for the server to close before calling `process.exit`. This lets it wait for any in-flight requests to finish processing before exiting the process - Sends `SIGKILL` to the child process in `next dev`, which should have the same effect of immediately shutting down the server on `SIGTERM` or `SIGINT` fixes: #53661 refs: #59551 ------ Previously #59551 attempted to fix #53661, but had broken some tests in the process. It looks like the final commit was also missing an intended change to `utils.ts`. This should fix those issues as well as introduce a new set of tests for the graceful shutdown feature. In the last PR I was squashing and force-pushing updates along the way but it made it difficult to track the changes. This time I'm pushing quite a few commits to make it easier to track the changes and refactors I've made, with the idea that this should be squashed before being merged.
2024-01-16 09:25:49 -08:00 · 2024-01-16 09:25:49 -08:00 · d08b3ffa50
commit d08b3ffa50
parent f668ab580b
10 changed files with 301 additions and 50 deletions
--- a/packages/next/src/bin/next.ts
+++ b/packages/next/src/bin/next.ts
@ -108,12 +108,11 @@ if (process.env.NODE_ENV) {
 ;(process.env as any).NODE_ENV = process.env.NODE_ENV || defaultEnv
 ;(process.env as any).NEXT_RUNTIME = 'nodejs'

-// Make sure commands gracefully respect termination signals (e.g. from Docker)
-// Allow the graceful termination to be manually configurable
-if (!process.env.NEXT_MANUAL_SIG_HANDLE && command !== 'dev') {
+if (command === 'build') {
  process.on('SIGTERM', () => process.exit(0))
  process.on('SIGINT', () => process.exit(0))
 }
+
 async function main() {
  const currentArgsSpec = commandArgs[command]()
  const validatedArgs = getValidatedArgs(currentArgsSpec, forwardedArgs)
--- a/packages/next/src/build/utils.ts
+++ b/packages/next/src/build/utils.ts
@ -2072,13 +2072,6 @@ const dir = path.join(__dirname)
 process.env.NODE_ENV = 'production'
 process.chdir(__dirname)

-// Make sure commands gracefully respect termination signals (e.g. from Docker)
-// Allow the graceful termination to be manually configurable
-if (!process.env.NEXT_MANUAL_SIG_HANDLE) {
-  process.on('SIGTERM', () => process.exit(0))
-  process.on('SIGINT', () => process.exit(0))
-}
-
 const currentPort = parseInt(process.env.PORT, 10) || 3000
 const hostname = process.env.HOSTNAME || '0.0.0.0'

--- a/packages/next/src/cli/next-dev.ts
+++ b/packages/next/src/cli/next-dev.ts
@ -28,27 +28,31 @@ import type { SelfSignedCertificate } from '../lib/mkcert'
 import uploadTrace from '../trace/upload-trace'
 import { initialEnv } from '@next/env'
 import { fork } from 'child_process'
+import type { ChildProcess } from 'child_process'
 import {
  getReservedPortExplanation,
  isPortIsReserved,
 } from '../lib/helpers/get-reserved-port'
 import os from 'os'
+import { once } from 'node:events'

 let dir: string
-let child: undefined | ReturnType<typeof fork>
+let child: undefined | ChildProcess
 let config: NextConfigComplete
 let isTurboSession = false
 let traceUploadUrl: string
 let sessionStopHandled = false
 let sessionStarted = Date.now()

-const handleSessionStop = async (signal: string | null) => {
-  if (child) {
-    child.kill((signal as any) || 0)
-  }
+const handleSessionStop = async (signal: NodeJS.Signals | number | null) => {
+  if (child?.pid) child.kill(signal ?? 0)
  if (sessionStopHandled) return
  sessionStopHandled = true

+  if (child?.pid && child.exitCode === null && child.signalCode === null) {
+    await once(child, 'exit').catch(() => {})
+  }
+
  try {
    const { eventCliSessionStopped } =
      require('../telemetry/events/session-stopped') as typeof import('../telemetry/events/session-stopped')
@ -107,8 +111,11 @@ const handleSessionStop = async (signal: string | null) => {
  process.exit(0)
 }

-process.on('SIGINT', () => handleSessionStop('SIGINT'))
-process.on('SIGTERM', () => handleSessionStop('SIGTERM'))
+process.on('SIGINT', () => handleSessionStop('SIGKILL'))
+process.on('SIGTERM', () => handleSessionStop('SIGKILL'))
+
+// exit event must be synchronous
+process.on('exit', () => child?.kill('SIGKILL'))

 const nextDev: CliCommand = async (args) => {
  if (args['--help']) {
@ -335,16 +342,4 @@ const nextDev: CliCommand = async (args) => {
  await runDevServer(false)
 }

-function cleanup() {
-  if (!child) {
-    return
-  }
-
-  child.kill('SIGTERM')
-}
-
-process.on('exit', cleanup)
-process.on('SIGINT', cleanup)
-process.on('SIGTERM', cleanup)
-
 export { nextDev }
--- a/packages/next/src/server/lib/start-server.ts
+++ b/packages/next/src/server/lib/start-server.ts
@ -264,10 +264,9 @@ export async function startServer(
      })

      try {
-        const cleanup = (code: number | null) => {
+        const cleanup = () => {
          debug('start-server process cleanup')
-          server.close()
-          process.exit(code ?? 0)
+          server.close(() => process.exit(0))
        }
        const exception = (err: Error) => {
          if (isPostpone(err)) {
@ -279,11 +278,11 @@ export async function startServer(
          // This is the render worker, we keep the process alive
          console.error(err)
        }
-        process.on('exit', (code) => cleanup(code))
+        // Make sure commands gracefully respect termination signals (e.g. from Docker)
+        // Allow the graceful termination to be manually configurable
        if (!process.env.NEXT_MANUAL_SIG_HANDLE) {
-          // callback value is signal string, exit with 0
-          process.on('SIGINT', () => cleanup(0))
-          process.on('SIGTERM', () => cleanup(0))
+          process.on('SIGINT', cleanup)
+          process.on('SIGTERM', cleanup)
        }
        process.on('rejectionHandled', () => {
          // It is ok to await a Promise late in Next.js as it allows for better
--- a/test/integration/telemetry/test/page-features.test.js
+++ b/test/integration/telemetry/test/page-features.test.js
@ -100,7 +100,7 @@ describe('page features telemetry', () => {
        await renderViaHTTP(port, '/hello')

        if (app) {
-          await killApp(app)
+          await killApp(app, 'SIGTERM')
        }
        await check(() => stderr, /NEXT_CLI_SESSION_STOPPED/)

@ -141,7 +141,7 @@ describe('page features telemetry', () => {
        await renderViaHTTP(port, '/hello')

        if (app) {
-          await killApp(app)
+          await killApp(app, 'SIGTERM')
        }

        await check(() => stderr, /NEXT_CLI_SESSION_STOPPED/)
--- a/test/lib/next-modes/base.ts
+++ b/test/lib/next-modes/base.ts
@ -10,6 +10,7 @@ import { Span } from 'next/src/trace'
 import webdriver from '../next-webdriver'
 import { renderViaHTTP, fetchViaHTTP, waitFor } from 'next-test-utils'
 import cheerio from 'cheerio'
+import { once } from 'events'
 import { BrowserInterface } from '../browsers/base'
 import escapeStringRegexp from 'escape-string-regexp'

@ -59,7 +60,7 @@ export class NextInstance {
  public testDir: string
  protected isStopping: boolean = false
  protected isDestroyed: boolean = false
-  protected childProcess: ChildProcess
+  protected childProcess?: ChildProcess
  protected _url: string
  protected _parsedUrl: URL
  protected packageJson?: PackageJson = {}
@ -331,13 +332,7 @@ export class NextInstance {
  public async stop(): Promise<void> {
    this.isStopping = true
    if (this.childProcess) {
-      let exitResolve
-      const exitPromise = new Promise((resolve) => {
-        exitResolve = resolve
-      })
-      this.childProcess.addListener('exit', () => {
-        exitResolve()
-      })
+      const exitPromise = once(this.childProcess, 'exit')
      await new Promise<void>((resolve) => {
        treeKill(this.childProcess.pid, 'SIGKILL', (err) => {
          if (err) {
--- a/test/lib/next-test-utils.ts
+++ b/test/lib/next-test-utils.ts
@ -17,6 +17,7 @@ import { getRandomPort } from 'get-port-please'
 import fetch from 'node-fetch'
 import qs from 'querystring'
 import treeKill from 'tree-kill'
+import { once } from 'events'

 import server from 'next/dist/server/next'
 import _pkg from 'next/package.json'
@ -497,7 +498,7 @@ export function buildTS(

 export async function killProcess(
  pid: number,
-  signal: string | number = 'SIGTERM'
+  signal: NodeJS.Signals | number = 'SIGTERM'
 ): Promise<void> {
  return await new Promise((resolve, reject) => {
    treeKill(pid, signal, (err) => {
@ -524,9 +525,18 @@ export async function killProcess(
 }

 // Kill a launched app
-export async function killApp(instance: ChildProcess) {
-  if (instance && instance.pid) {
-    await killProcess(instance.pid)
+export async function killApp(
+  instance?: ChildProcess,
+  signal: NodeJS.Signals | number = 'SIGKILL'
+) {
+  if (
+    instance?.pid &&
+    instance.exitCode === null &&
+    instance.signalCode === null
+  ) {
+    const exitPromise = once(instance, 'exit')
+    await killProcess(instance.pid, signal)
+    await exitPromise
  }
 }

--- a/test/production/graceful-shutdown/index.test.ts
+++ b/test/production/graceful-shutdown/index.test.ts
@ -0,0 +1,236 @@
+import { join } from 'path'
+import { NextInstance, createNext, FileRef } from 'e2e-utils'
+import {
+  fetchViaHTTP,
+  findPort,
+  initNextServerScript,
+  killApp,
+  launchApp,
+  nextBuild,
+  nextStart,
+  waitFor,
+} from 'next-test-utils'
+import fs from 'fs-extra'
+import glob from 'glob'
+import { LONG_RUNNING_MS } from './src/pages/api/long-running'
+import { once } from 'events'
+
+const appDir = join(__dirname, './src')
+let appPort
+let app
+
+function assertDefined<T>(value: T | void): asserts value is T {
+  expect(value).toBeDefined()
+}
+
+describe('Graceful Shutdown', () => {
+  describe('development (next dev)', () => {
+    beforeEach(async () => {
+      appPort = await findPort()
+      app = await launchApp(appDir, appPort)
+    })
+    afterEach(() => killApp(app))
+
+    runTests(true)
+  })
+  ;(process.env.TURBOPACK ? describe.skip : describe)(
+    'production (next start)',
+    () => {
+      beforeAll(async () => {
+        await nextBuild(appDir)
+      })
+      beforeEach(async () => {
+        appPort = await findPort()
+        app = await nextStart(appDir, appPort)
+      })
+      afterEach(() => killApp(app))
+
+      runTests()
+    }
+  )
+  ;(process.env.TURBOPACK ? describe.skip : describe)(
+    'production (standalone mode)',
+    () => {
+      let next: NextInstance
+      let serverFile
+
+      const projectFiles = {
+        'next.config.mjs': `export default { output: 'standalone' }`,
+      }
+
+      for (const file of glob.sync('*', { cwd: appDir, dot: false })) {
+        projectFiles[file] = new FileRef(join(appDir, file))
+      }
+
+      beforeAll(async () => {
+        next = await createNext({
+          files: projectFiles,
+          dependencies: {
+            swr: 'latest',
+          },
+        })
+
+        await next.stop()
+
+        await fs.move(
+          join(next.testDir, '.next/standalone'),
+          join(next.testDir, 'standalone')
+        )
+
+        for (const file of await fs.readdir(next.testDir)) {
+          if (file !== 'standalone') {
+            await fs.remove(join(next.testDir, file))
+          }
+        }
+        const files = glob.sync('**/*', {
+          cwd: join(next.testDir, 'standalone/.next/server/pages'),
+          dot: true,
+        })
+
+        for (const file of files) {
+          if (file.endsWith('.json') || file.endsWith('.html')) {
+            await fs.remove(join(next.testDir, '.next/server', file))
+          }
+        }
+
+        serverFile = join(next.testDir, 'standalone/server.js')
+      })
+
+      beforeEach(async () => {
+        appPort = await findPort()
+        app = await initNextServerScript(
+          serverFile,
+          /- Local:/,
+          { ...process.env, PORT: appPort.toString() },
+          undefined,
+          { cwd: next.testDir }
+        )
+      })
+      afterEach(() => killApp(app))
+
+      afterAll(() => next.destroy())
+
+      runTests()
+    }
+  )
+})
+
+function runTests(dev = false) {
+  if (dev) {
+    it('should shut down child immediately', async () => {
+      const appKilledPromise = once(app, 'exit')
+
+      // let the dev server compile the route before running the test
+      await expect(
+        fetchViaHTTP(appPort, '/api/long-running')
+      ).resolves.toBeDefined()
+
+      const resPromise = fetchViaHTTP(appPort, '/api/long-running')
+
+      // yield event loop to kick off request before killing the app
+      await waitFor(20)
+      process.kill(app.pid, 'SIGTERM')
+      expect(app.exitCode).toBe(null)
+
+      // `next dev` should kill the child immediately
+      let start = Date.now()
+      await expect(resPromise).rejects.toThrow()
+      expect(Date.now() - start).toBeLessThan(LONG_RUNNING_MS)
+
+      // `next dev` parent process is still running cleanup
+      expect(app.exitCode).toBe(null)
+
+      // App finally shuts down
+      await appKilledPromise
+      expect(app.exitCode).toBe(0)
+    })
+  } else {
+    it('should wait for requests to complete before exiting', async () => {
+      const appKilledPromise = once(app, 'exit')
+
+      let responseResolved = false
+      const resPromise = fetchViaHTTP(appPort, '/api/long-running')
+        .then((res) => {
+          responseResolved = true
+          return res
+        })
+        .catch(() => {})
+
+      // yield event loop to kick off request before killing the app
+      await waitFor(20)
+      process.kill(app.pid, 'SIGTERM')
+      expect(app.exitCode).toBe(null)
+
+      // Long running response should still be running after a bit
+      await waitFor(LONG_RUNNING_MS / 2)
+      expect(app.exitCode).toBe(null)
+      expect(responseResolved).toBe(false)
+
+      // App responds as expected without being interrupted
+      const res = await resPromise
+      assertDefined(res)
+      expect(res.status).toBe(200)
+      expect(await res.json()).toStrictEqual({ hello: 'world' })
+
+      // App is still running briefly after response returns
+      expect(app.exitCode).toBe(null)
+      expect(responseResolved).toBe(true)
+
+      // App finally shuts down
+      await appKilledPromise
+      expect(app.exitCode).toBe(0)
+    })
+
+    describe('should not accept new requests during shutdown cleanup', () => {
+      it('when request is made before shutdown', async () => {
+        const appKilledPromise = once(app, 'exit')
+
+        const resPromise = fetchViaHTTP(appPort, '/api/long-running')
+
+        // yield event loop to kick off request before killing the app
+        await waitFor(20)
+        process.kill(app.pid, 'SIGTERM')
+        expect(app.exitCode).toBe(null)
+
+        // Long running response should still be running after a bit
+        await waitFor(LONG_RUNNING_MS / 2)
+        expect(app.exitCode).toBe(null)
+
+        // Second request should be rejected
+        await expect(
+          fetchViaHTTP(appPort, '/api/long-running')
+        ).rejects.toThrow()
+
+        // Original request responds as expected without being interrupted
+        await expect(resPromise).resolves.toBeDefined()
+        const res = await resPromise
+        expect(res.status).toBe(200)
+        expect(await res.json()).toStrictEqual({ hello: 'world' })
+
+        // App is still running briefly after response returns
+        expect(app.exitCode).toBe(null)
+
+        // App finally shuts down
+        await appKilledPromise
+        expect(app.exitCode).toBe(0)
+      })
+
+      it('when there is no activity', async () => {
+        const appKilledPromise = once(app, 'exit')
+
+        process.kill(app.pid, 'SIGTERM')
+        expect(app.exitCode).toBe(null)
+
+        // yield event loop to allow server to start the shutdown process
+        await waitFor(20)
+        await expect(
+          fetchViaHTTP(appPort, '/api/long-running')
+        ).rejects.toThrow()
+
+        // App finally shuts down
+        await appKilledPromise
+        expect(app.exitCode).toBe(0)
+      })
+    })
+  }
+}
--- a/test/production/graceful-shutdown/src/pages/api/long-running.ts
+++ b/test/production/graceful-shutdown/src/pages/api/long-running.ts
@ -0,0 +1,6 @@
+export const LONG_RUNNING_MS = 400
+
+export default async (req, res) => {
+  await new Promise((resolve) => setTimeout(resolve, LONG_RUNNING_MS))
+  res.json({ hello: 'world' })
+}
--- a/test/production/graceful-shutdown/src/tsconfig.json
+++ b/test/production/graceful-shutdown/src/tsconfig.json
@ -0,0 +1,18 @@
+{
+  "compilerOptions": {
+    "lib": ["dom", "dom.iterable", "esnext"],
+    "allowJs": true,
+    "skipLibCheck": true,
+    "strict": false,
+    "noEmit": true,
+    "incremental": true,
+    "esModuleInterop": true,
+    "module": "esnext",
+    "moduleResolution": "node",
+    "resolveJsonModule": true,
+    "isolatedModules": true,
+    "jsx": "preserve"
+  },
+  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx"],
+  "exclude": ["node_modules"]
+}