Skip to content

Fixed potential bugs due to incomplete signal handling #81699

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

Open
wants to merge 2 commits into
base: canary
Choose a base branch
from

Conversation

suwakei
Copy link

@suwakei suwakei commented Jul 16, 2025

Problem

1. potential bug: incomplete signal handling:

The current code calls process.exit() directly upon receiving an exit signal such as SIGTERM or SIGINT.

// next.js/packages/next/src/cli/next-build.ts:32-33
process.on('SIGTERM', () => process.exit(143))
process.on('SIGINT', () => process.exit(130))

Calling process.exit() terminates the Node.js process immediately.
This does not guarantee completion of asynchronous processing or execution of the finally block. As a result,
disableMemoryDebuggingMode() may not be called when externalDebugMemoryUsage is enabled and resources may not be released.

2. code duplication: redundant cleanup process:

cleanup process (call to disableMemoryDebuggingMode()) exists in both .catch() and .finally() blocks.

// next.js/packages/next/src/cli/next-build.ts:125-144
    .catch((err) => {
      if (experimentalDebugMemoryUsage) {
        disableMemoryDebuggingMode() // ← here
      }
      // ...
    })
    .finally(() => {
      if (experimentalDebugMemoryUsage) {
        disableMemoryDebuggingMode() // ← and, here
      }
    })

Since the .finally block is always executed regardless of the success or failure of the Promise, I thought the cleanup process in .catch() was redundant.

Proposal

1. Combine cleanup logic into a function:

encapsulate the cleanup process as a cleanup function to eliminate code duplication.

  const cleanup = () => {
    if (experimentalDebugMemoryUsage) {
      disableMemoryDebuggingMode()
    }
  }

2. Improve the signal handler:

Modify it so that when it receives an exit signal,
it first calls the cleanup function before terminating the process.
This will ensure that cleanup is also performed when a signal is received.

  process.on('SIGTERM', () => {
    cleanup()
    process.exit(143)
  })
  process.on('SIGINT', () => {
    cleanup()
    process.exit(130)
  })

3. Simplify the Promise chain:

.finally(cleanup) to remove redundant calls in .catch().

   traceUploadUrl
  )
    .catch((err) => {
      --- if (experimentalDebugMemoryUsage) {
        --- disableMemoryDebuggingMode()
      --- }
      console.error('')
      if (
        isError(err) &&
        printAndExit(err)
      }
    })
    --- .finally(() => {
      --- if (experimentalDebugMemoryUsage) {
        --- disableMemoryDebuggingMode()
      --- }
    --- })
    +++ .finally(cleanup)
}

export { nextBuild }

This change ensures that no matter under what circumstances a process exits,
if the memory debugging feature is enabled, it will always disable it and ensure that it exits clean.
It can then play an important role in preventing resource leaks and stabilizing the system.

@ijjk
Copy link
Member

ijjk commented Jul 16, 2025

Allow CI Workflow Run

  • approve CI run for commit: 5a6dcb6

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants