Skip to content

fix: gracefully shutdown server #59551

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.

Sign up for GitHub

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

Merged
huozhi merged 1 commit into from
Dec 19, 2023
Merged

fix: gracefully shutdown server #59551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions packages/next/src/cli/next-dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,20 @@ import {
} from '../lib/helpers/get-reserved-port'
import os from 'os'

type Child = ReturnType<typeof fork>
type ExitCode = 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: ExitCode | null) => {
if (child) {
child.kill((signal as any) || 0)
child.kill(signal ?? 0)
}
if (sessionStopHandled) return
sessionStopHandled = true
Expand Down Expand Up @@ -108,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'))
Copy link
Contributor Author

@redbmk redbmk Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed these to SIGKILL to make sure the child process stops immediately. In a production server we would want to pass along the SIGINT or SIGTERM but if you're killing the dev server you probably wouldn't expect it to do any cleanup. This should also be consistent with the current behavior that exits right away with process.exit


const nextDev: CliCommand = async (args) => {
if (args['--help']) {
Expand Down Expand Up @@ -333,16 +339,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)
Copy link
Contributor Author

@redbmk redbmk Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other handlers were already killing the child - this one always sends SIGTERM. However, we still need to add a special handler for the exit event since it needs to be synchronous


export { nextDev }
13 changes: 6 additions & 7 deletions packages/next/src/server/lib/start-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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))
Copy link
Contributor Author

@redbmk redbmk Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://nodejs.org/api/process.html#event-exit

Listener functions must only perform synchronous operations. The Node.js process will exit immediately after calling the 'exit' event listeners causing any additional work still queued in the event loop to be abandoned.

Once process.exit is called we don't really have a chance to clean up since server.close is asynchronous. Best we could do is log a message, but not sure that's very useful.

Also calling the same cleanup code, now that there's a log, was resulting in the log printing twice after a signal was received.

// 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
Expand Down
2 changes: 1 addition & 1 deletion test/lib/next-test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ export async function killProcess(
// Kill a launched app
export async function killApp(instance: ChildProcess) {
if (instance && instance.pid) {
await killProcess(instance.pid)
await killProcess(instance.pid, 'SIGKILL')
}
}

Expand Down