Skip to content
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

Support graceful shutdown after receiving SIGTERM signal #29959

Closed

Conversation

banksalad-seongwoo
Copy link

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number Graceful shutdown #19693
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes

process.on('SIGTERM', () => process.exit(0))
process.on('SIGTERM', () =>
setTimeout(
() => process.exit(0),

Choose a reason for hiding this comment

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

Is there anyway that this could be something like:

() => {
  server.close();
  process.exit(0);
}

@aerott
Copy link

aerott commented Feb 9, 2022

Hello. Why does the introducing of this change take so long? :( It seems not to be a big deal, but it should be helpful for running Next app in Kubernetes cluster.

@@ -94,7 +94,12 @@ if (process.env.NODE_ENV && !standardEnv.includes(process.env.NODE_ENV)) {
;(process.env as any).NODE_ENV = process.env.NODE_ENV || defaultEnv

// Make sure commands gracefully respect termination signals (e.g. from Docker)
process.on('SIGTERM', () => process.exit(0))
process.on('SIGTERM', () =>
Copy link
Member

@ijjk ijjk Feb 10, 2022

Choose a reason for hiding this comment

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

What do you think about these listeners being omitted when an environment variable is present and instead adding custom handling for this in _document?

This would allow more control over calling process.exit() instead of relying on an arbitrary time value. e.g.

if (!process.env.NEXT_MANUAL_SIGTERM) {
  process.on('SIGTERM', () => process.exit(0))
}
// pages/_document.js

if (process.env.NEXT_MANUAL_SIGTERM) {
  // this should be added in your custom _document 
  process.on('SIGTERM', () => {
    console.log('cleaning up')
    process.exit(0)
  })
}

Copy link

Choose a reason for hiding this comment

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

love this, we would like to overwrite this to close database connection ...

@nicolas-vivot
Copy link

nicolas-vivot commented Mar 10, 2022

Handling correctly SIGTERM to gracefully shutdown the next server should be a day 1 mandatory feature.

It's amazing that this is still not supported and that the only option to go with is a customized server, loosing some benefit of next (optimization)

@aerott

This comment was marked as spam.

@sidwebworks
Copy link
Contributor

@leerob Bro you got any ideas as to why this issue is still stale? Most of us are happy to help if there's something required from the community.

@ijjk
Copy link
Member

ijjk commented May 13, 2022

@sidwebworks we're definitely open to landing this change seems this PR has gone stale so if you would like to open a fresh one implementing the change requested here #29959 (comment) that would be great.

@sidwebworks
Copy link
Contributor

@ijjk Hey, I have created a fresh PR and requested for a code review here #36909

@ijjk ijjk mentioned this pull request May 15, 2022
9 tasks
@kodiakhq kodiakhq bot closed this in #36909 May 16, 2022
kodiakhq bot pushed a commit that referenced this pull request May 16, 2022
## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [x] Related issues linked using [19693](#19693)
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ x] Make sure the linting passes by running `yarn lint`


Closes #29959
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants