-
Notifications
You must be signed in to change notification settings - Fork 27.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
graceful shutdown #60059
graceful shutdown #60059
Changes from 18 commits
5b004f2
57f880a
e3b0427
7d94719
7c8ff12
c739e11
4f74aee
90af3bb
f3c875d
8de8407
b173496
df3a4e5
c40330a
778b5dc
49af857
2140702
5622b51
e2a7700
44fe46a
8111659
8e273b8
e340c41
3f65ded
e49fb6d
c43fc86
0c0c73e
712643c
77bed79
aacc91c
c4af87d
f7faf58
e88a95b
fd84659
b14eab2
ec24f76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still want to stop the CLI immediately if SIGTERM or SIGINT is used for building. There's also a test for this already that was breaking if we remove these handlers for all commands. |
||
process.on('SIGTERM', () => process.exit(0)) | ||
process.on('SIGINT', () => process.exit(0)) | ||
} | ||
|
||
async function main() { | ||
const currentArgsSpec = commandArgs[command]() | ||
const validatedArgs = getValidatedArgs(currentArgsSpec, forwardedArgs) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2054,13 +2054,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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing this one is necessary for standalone mode to gracefully shutdown. The env is still in |
||
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' | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,22 +33,33 @@ import { | |
isPortIsReserved, | ||
} from '../lib/helpers/get-reserved-port' | ||
import os from 'os' | ||
import { once } from 'node:events' | ||
|
||
type Child = ReturnType<typeof fork> | ||
type KillSignal = Parameters<Child['kill']>[0] | ||
|
||
let dir: string | ||
let child: undefined | ReturnType<typeof fork> | ||
let child: undefined | Child | ||
let config: NextConfigComplete | ||
let isTurboSession = false | ||
let traceUploadUrl: string | ||
let sessionStopHandled = false | ||
let sessionStarted = Date.now() | ||
|
||
const handleSessionStop = async (signal: string | null) => { | ||
const handleSessionStop = async ( | ||
signal: KillSignal | null, | ||
childExited: boolean = false | ||
) => { | ||
if (child) { | ||
child.kill((signal as any) || 0) | ||
child.kill(signal ?? 0) | ||
} | ||
if (sessionStopHandled) return | ||
sessionStopHandled = true | ||
|
||
if (child && !childExited) { | ||
await once(child, 'exit').catch(() => {}) | ||
} | ||
|
||
try { | ||
const { eventCliSessionStopped } = | ||
require('../telemetry/events/session-stopped') as typeof import('../telemetry/events/session-stopped') | ||
|
@@ -108,8 +119,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')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a little unsure about switching this to SIGKILL because it could potentially mess with someone using a custom signal but it looks like that was never supported in https://nextjs.org/docs/pages/building-your-application/deploying
Also because of the block in So because of all that I don't believe this would be a breaking change, and should still be backwards compatible |
||
|
||
// exit event must be synchronous | ||
process.on('exit', () => child?.kill('SIGKILL')) | ||
|
||
const nextDev: CliCommand = async (args) => { | ||
if (args['--help']) { | ||
|
@@ -287,7 +301,7 @@ const nextDev: CliCommand = async (args) => { | |
} | ||
return startServer(options) | ||
} | ||
await handleSessionStop(signal) | ||
await handleSessionStop(signal, true) | ||
}) | ||
}) | ||
} | ||
|
@@ -333,16 +347,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 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we don't need to handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Since https://nodejs.org/api/net.html#serverclosecallback
I could see there maybe being a couple things that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also just realized it was already using the original exit code and just defaulting to 0, so the behavior should still be the same without listening for the event |
||
// 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { remove } from 'fs-extra' | |
import { | ||
findPort, | ||
killApp, | ||
killProcess, | ||
launchApp, | ||
nextBuild, | ||
nextStart, | ||
|
@@ -180,6 +181,8 @@ describe('CSS Module client-side navigation', () => { | |
}) | ||
afterAll(async () => { | ||
proxyServer.close() | ||
// something is hanging onto the process, so we need to SIGKILL | ||
await killProcess(app.pid, 'SIGKILL') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't able to track down why the app wouldn't close. I tried closing Using |
||
await killApp(app) | ||
}) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import { Span } from 'next/src/trace' | |
import webdriver from '../next-webdriver' | ||
import { renderViaHTTP, fetchViaHTTP } 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 = {} | ||
|
@@ -345,13 +346,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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had started making changes here and turned out they weren't needed, but I thought I'd keep this refactor since |
||
await new Promise<void>((resolve) => { | ||
treeKill(this.childProcess.pid, 'SIGKILL', (err) => { | ||
if (err) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -523,10 +524,24 @@ export async function killProcess( | |
}) | ||
} | ||
|
||
export function isAppRunning(instance: ChildProcess) { | ||
if (!instance?.pid) return false | ||
|
||
try { | ||
// 0 is a special signal that tests the existence of a process | ||
process.kill(instance.pid, 0) | ||
return true | ||
} catch { | ||
return false | ||
} | ||
} | ||
|
||
// Kill a launched app | ||
export async function killApp(instance: ChildProcess) { | ||
if (instance && instance.pid) { | ||
if (instance?.pid && isAppRunning(instance)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to kill the app if it's already running. |
||
const exitPromise = once(instance, 'exit') | ||
await killProcess(instance.pid) | ||
await exitPromise | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this one is necessary for
next dev
andnext start
to gracefully shutdown. The env is still instart-server