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

Nest-cli depends on tree-kill, which depends on ps command, which may be unavailable and is not documented #484

Open
OJezu opened this issue Nov 26, 2019 · 12 comments

Comments

@OJezu
Copy link

OJezu commented Nov 26, 2019

I'm submitting a...


[ ] Regression 
[X] Bug report
[ ] Feature request
[X] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

When application is started with yarn run start:dev inside a container with no ps program available, after first code change application crashes completely (along with the container)

$ yarn run start:dev
5:39:37 PM - File change detected. Starting incremental compilation...


5:39:39 PM - Found 0 errors. Watching for file changes.
events.js:186
      throw er; // Unhandled 'error' event
      ^

Error: spawn ps ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:264:19)
    at onErrorNT (internal/child_process.js:456:16)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)
Emitted 'error' event on ChildProcess instance at:
    at Process.ChildProcess._handle.onexit (internal/child_process.js:270:12)
    at onErrorNT (internal/child_process.js:456:16)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  errno: 'ENOENT',
  code: 'ENOENT',
  syscall: 'spawn ps',
  path: 'ps',
  spawnargs: [ '-o', 'pid', '--no-headers', '--ppid', 185 ]
}

Expected behavior

Application reloads with new code changes

Minimal reproduction of the problem with instructions

  1. Create new docker container from node image
  2. Create new app with nest-cli inside the container
  3. Start the app with npm run start:dev
  4. Change something in application code
  5. npm run start:dev crashes completely

What is the motivation / use case for changing the behavior?

Unexpected crashes.

Possible fixes:

  • dont' use tree-kill, make child a detached process with detach option, and use process.kill(-childrenPid) to kill entire process tree, faff around with signals and stdio to make sure child process' tree is killed when needed https://nodejs.org/api/child_process.html#child_process_options_detached
  • add ps dependency to documentation
  • check for ps being available and print sensible error
  • ask nicely tree-kill to output more legible error when no ps is available

Environment

 _   _             _      ___  _____  _____  _     _____
| \ | |           | |    |_  |/  ___|/  __ \| |   |_   _|
|  \| |  ___  ___ | |_     | |\ `--. | /  \/| |     | |
| . ` | / _ \/ __|| __|    | | `--. \| |    | |     | |
| |\  ||  __/\__ \| |_ /\__/ //\__/ /| \__/\| |_____| |_
\_| \_/ \___||___/ \__|\____/ \____/  \____/\_____/\___/


[System Information]
OS Version     : Linux 5.0
NodeJS Version : v12.10.0
YARN Version    : 1.19.2
[Nest Information]
platform-express version : 6.7.2
typeorm version          : 6.2.0
common version           : 6.7.2
core version             : 6.7.2

@kamilmysliwiec
Copy link
Member

ask nicely tree-kill to output more legible error when no ps is available

Would you like to create an issue in the tree-kill repo?

  • add ps dependency to documentation
  • check for ps being available and print sensible error

We can do both on our side ^

@b4dnewz
Copy link

b4dnewz commented Dec 13, 2019

I've encountered this problem right now, @kamilmysliwiec if you are interested I can make a PR (to nestjs) with the first proposed solution:

dont' use tree-kill, make child a detached process with detach option

I'm running nest in dev mode inside a docker container because I'm interacting with other services and it's a features/fix that I really need

@OJezu
Copy link
Author

OJezu commented Dec 16, 2019

@b4dnewz not sure it will work on Windows though, might still have to use taskkill program there.
Also, for UNIX-like, if children start their own detached children, those grandchildren won't be hunted down and killed. However this may be an expected outcome - after all they are detached from their parent, so they should not be auto-killed when parent dies.

@b4dnewz
Copy link

b4dnewz commented Dec 16, 2019

@OJezu good point

if children start their own detached children

it this the case for nestjs start:dev? i'm not familiar with how it's working in deep but looks like it will spawn one per time process after the build has completed successfully

not sure it will work on Windows though

I'm not sure either but after looking round in github issues I found this comment and this comment in the same page, which may lead to a nice and clean solution, I still haven't tested

In particular the second comment could solve the issue of children and grandchildren

@OJezu
Copy link
Author

OJezu commented Dec 16, 2019

@b4dnewz start:dev starts the application, if that application starts it's own children is not up to nest.

The comments below mention, that killing process group (with kill(-pid)) works on non-windows. I don't see nothing there about killing process groups on windows.

@b4dnewz
Copy link

b4dnewz commented Dec 16, 2019

yep you right, I misunderstood the comment, my english understanding sucks..

@jexperton
Copy link

jexperton commented Apr 20, 2020

I just spent a couple of days investigating this issue, including unsuccessfully asking for information on Discord.

I've been working on other projects with ts-node-dev which does the same as start:dev so I knew it could work with Docker.

After digging into the nest-cli code, I've found the killProcess function.

This made me realized that I should have clicked on this slightly misleading issue title when I first saw it in the issue list two days ago.

Don't use official debian slim node images. Use debian full or alpine images

+1 for a better documentation.

@OJezu
Copy link
Author

OJezu commented Apr 21, 2020

@jexperton re: the issue titile
If you were to look up the error message using issue search, you would find this issue. I'm sorry if the title is not searchable, but I'd rather have the title describe the issue, not the symptom. For searching... well, use search, GitHub's, or Google.

@jexperton
Copy link

jexperton commented Apr 21, 2020

@OJezu you're right I should blame my search skills not the issue title. That's why I agree with documenting this issue. It could be interesting to add a short advice such as:

If you use Docker for development, make sure that your image has ps installed by running the following command:

docker run --rm --entrypoint ps YOUR_IMAGE &> /dev/null && echo "installed" || echo "missing"

@jmlopez-rod
Copy link

Ran into this issue today. After installing ps on my docker image it works:

FROM node:12.18.3-buster-slim

RUN apt-get update && apt-get install -y procps

I think a note on using Docker like the one @jexperton suggested would help.

@dimacros
Copy link

Lol, this is surely a common problem for those who use Docker.

@befabry
Copy link

befabry commented May 17, 2022

Still running in this issue. Quite annoying but @jexperton and @jmlopez-rod workarounds are good enough !

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

No branches or pull requests

7 participants