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

Close http server in cleanup #53661

Closed
1 task done
Meemaw opened this issue Aug 7, 2023 · 2 comments · Fixed by #59551 or #60059
Closed
1 task done

Close http server in cleanup #53661

Meemaw opened this issue Aug 7, 2023 · 2 comments · Fixed by #59551 or #60059
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Output Related to the the output configuration option.

Comments

@Meemaw
Copy link

Meemaw commented Aug 7, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

    Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000
    Binaries:
      Node: 20.3.1
      npm: 9.6.7
      Yarn: 1.22.18
      pnpm: 7.29.3
    Relevant Packages:
      next: 13.4.13-canary.18
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.3
    Next.js Config:
      output: N/A

Which area(s) of Next.js are affected? (leave empty if unsure)

Standalone mode (output: "standalone")

Link to the code that reproduces this issue or a replay of the bug

https://github.com/Meemaw/next-server-shutdown-reproducer

To Reproduce

  1. yarn build
  2. yarn start
  3. Visit http://localhost:3000
  4. Close the server before the page renders
  5. Get a "This site can’t be reached" error

Describe the Bug

NextJS doesn't gracefully handle inflight HTTP server connections. There is a cleanup code

, but it doesn't call server.cleanup().

There also seems to be a teardown function defined

async function teardown() {
)

, which does the correct thing, but is never called?

Expected Behavior

server.cleanup() should be called & awaited so any inflight requests are gracefully handled.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-1961

@Meemaw Meemaw added the bug Issue was opened via the bug report template. label Aug 7, 2023
@github-actions github-actions bot added the Output Related to the the output configuration option. label Aug 7, 2023
@redbmk
Copy link
Contributor

redbmk commented Dec 13, 2023

It looks like the code has change a bit but the issue is still there. To add to this, it looks like the standalone server.js file that gets generated has it's own conflicting code that immediately kills the server.

// 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))
}

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))
}

So as I understand this, the standalone server.js will run process.exit(0) unless you have the NEXT_MANUAL_SIG_HANDLE flag set, but then it calls startServer which will run server.close and then process.exit unless you set the same variable.

I would think since start-server has this code, we actually shouldn't have it in server.js

redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 13, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 14, 2023
- 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
- Let dev server pass `SIGINT`/`SIGKILL` to next-server to do
  appropriate cleanup

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 15, 2023
- 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
- Let dev server pass `SIGINT`/`SIGKILL` to next-server to do
  appropriate cleanup

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 17, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 17, 2023
- 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

fixes: vercel#53661
redbmk added a commit to redbmk/next.js that referenced this issue Dec 18, 2023
- 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

fixes: vercel#53661
@huozhi huozhi reopened this Dec 20, 2023
@huozhi huozhi added the linear: next Confirmed issue that is tracked by the Next.js team. label Jan 3, 2024
@huozhi huozhi closed this as completed in d08b3ff Jan 16, 2024
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Output Related to the the output configuration option.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants