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

Docker example wraps the next CLI with yarn in CMD and will not receive process signals (e.g. from Kubernetes) #29023

Closed
stefee opened this issue Sep 11, 2021 · 1 comment · Fixed by #29024
Labels
bug Issue was opened via the bug report template. examples Issue/PR related to examples

Comments

@stefee
Copy link
Contributor

stefee commented Sep 11, 2021

What example does this report relate to?

with-docker

What version of Next.js are you using?

latest

What version of Node.js are you using?

N/A

What browser are you using?

N/A

What operating system are you using?

N/A

How are you deploying your application?

Docker

Describe the Bug

The with-docker example and Deployment with Docker Image docs use similar Dockerfile example.

The Dockerfile has CMD as yarn start

CMD ["yarn", "start"]

This means that yarn will be used as default entrypoint executable when running the container.

As a result if a container manager such as Kubernetes sends process signals (SIGTERM/SIGINT) to the container created from this Dockerfile, the yarn process will receive these signals and expected to handle them correctly. There is no guarantee that yarn will forward these signals correctly to the child process (in this case, next CLI).

Without receiving the process signals from the Docker manager, next CLI is unable to respond appropriately to the context in which it is run - for example, to wait for in-flight HTTP requests to be fulfilled before exiting.

Also, we should consider that users will copy this example and use it with npm - which also has no guarantee to forward process signals to the child process from start or run-script.

A user might also decide to use next CLI initially and then change to use a custom Node.js server inside their Docker container (e.g. by changing their start script in package.json to use node index.js instead of next start). In this case, they might not realise that the node process is not able to receive signals from the container manager.

These are some of the reasons why it is recommended by Node.js Docker WG and Snyk that you do not use npm or yarn script as the entrypoint for a container.

Expected Behavior

Docker example could use the following instead:

ENV PORT 3000

CMD ["node_modules/.bin/next", "start"]

Sidenote:

The deployment guide could also mention that if you are using a custom Node.js server in Docker, then the node process will not handle SIGTERM and SIGINT signals sent via the container by default when it is run as container entrypoint [source], and recommend using docker run --init flag, using a small init binary like tini as the entrypoint instead, or handling process signals in code using process.on or a package like terminus or lightship.

To Reproduce

$ cd examples/with-docker
$ yarn  # There is no yarn.lock in the repo, we must generate it first
$ docker build . --tag=next-with-docker
$ docker run -p 3000:3000 next-with-docker

# Expected: "ready - started server on 0.0.0.0:3000, url: http://localhost:3000"
$ docker kill --signal="SIGINT" <container name>  # OR --signal="SIGTERM"

# Expected: server will stop gracefully

Note: the above should be the same whether using yarn, npm, next CLI or a custom Node.js server.

@stefee stefee added bug Issue was opened via the bug report template. examples Issue/PR related to examples labels Sep 11, 2021
@kodiakhq kodiakhq bot closed this as completed in #29024 Nov 1, 2021
kodiakhq bot pushed a commit that referenced this issue Nov 1, 2021
Fixes #29023 

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [x] 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`
- [ ] 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
timneutkens pushed a commit to timneutkens/next.js that referenced this issue Nov 2, 2021
…9024)

Fixes vercel#29023 

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [x] 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`
- [ ] 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
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
…9024)

Fixes vercel#29023 

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [x] 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`
- [ ] 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
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. examples Issue/PR related to examples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants