-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Nodemon waits forever for subprocesses to end #1633
Nodemon waits forever for subprocesses to end #1633
Comments
I'm able to confirm a similar issue in the following setup:
|
Investigating now. Also wanted to say @jayseeare, thank you for providing a repo to replicate on - this makes things much easier ❤️ |
Confirming this isn't an issue on Mac (my default setup). Now testing Ubuntu so I can replicate. |
Confirmed. Am debugging now. |
Source of this problem is that the PR #1579 is trying to kill non-existent processes:
Note the |
Thanks @remy. Sorry I didn't get round to debugging it last night; glad the repo helped though. |
Quick update, I think I've got a fix ready to land. |
If you install I've tested on:
All working as I would expect now. If someone can validate this with the debug release, I can publish. |
Testing |
Fixed for me on Mac Mojave, although I am getting this error in console: [nodemon] restarting due to changes...
error Command failed with signal "SIGUSR2".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. Doesn't seem to affect behaviour though |
Hmm, it shouldn't be doing that. @julienvincent can you run the same test with |
@remy Tested, can confirm I don't get that error with |
@julienvincent sod. Are you able to share how you're testing? I've got Mojave so I should be able to replicate. I'm using npm so I guess the first test is to use yarn. |
What's odd here is that SIGUSR2 isn't actually sent to the process, on the Mac it should be trimmed to USR2… |
Internal repository so can't share the codebase, but I am running it through yarn: package.json {
"scripts": {
"start": "yarn build && env-cmd node ./dist/index.js",
"watch": "nodemon --exec 'yarn start'"
}
} nodemon.json {
"ignore": ["dist", "types/dist", "types/schema"],
"watch": ["src", "types/src"],
"ext": "ts,json"
} Possible that |
Oh possible. Are you able to reduce the example? |
Ok after a bit of debugging I have narrowed it down to: {
"scripts": {
"watch": "nodemon --exec './node_modules/.bin/tsc -b && node ./dist/index.js'"
}
} Which produces So it looks like Tested with typescript |
@julienvincent and you're on a mac, right? No in docker or anything? |
Direct on mac, yea |
@julienvincent sorry, I don't do any typescript, so I'm hitting a few issues that I don't understand. My simple ts file is this: import http from 'http'
http.createServer(() => {}).listen(4000); And |
You have to use one of the following: import * as http from 'http';
http.createServer(() => {}).listen(4000); or import { createServer } from 'http';
createServer(() => {}).listen(4000); |
Huh. Still complains:
|
Let me setup a quick reproduce repo |
Ta - I'm about to shoot to an appointment but I'll be able to pick up in the evening - I really appreciate the repo and help. |
https://github.com/julienvincent/nodemon-1633-reproduce Kinda hacky, but you can reproduce this by running I also noticed that while using nodemon |
Just in case you're interested @remy: |
I've now tested with and without typescript, using @julienvincent's repo, and without typescript, there's no warning. It's only with typescript… :sigh: |
Actually - no it's not typescript, it's something to do with running more than one command in the |
I can replicate with this:
Then hit I'm going to check this with nodemon@1 and if has the same error, I'm going to ship and separate this echo thing into a separate bug. |
dangit. nodemon@1 doesn't do it. Poop. |
Making progress. It's a system level problem, this replicates it (on a mac, and I suspect, on linux): sh -c "sh -c sleep 20" & kill -USR2 $!
[1] 52500
[1] + 52500 user-defined signal 2 sh -c "sh -c sleep 20" Which is what nodemon is effectively doing. |
Confirmed it works on |
@kilianc confirms the bug occurs or that the fix works? |
Can anyone on this thread reinstall nodemon@debug - should be 2.0.1-alpha.2 - I believe I've got the core issue fixed and the shell error also fixed, but it needs a few different ways of hitting to be sure. |
@remy that the fix works! |
@kilianc can you test with the latest @ debug version? I'm worried that I'm picking inside the core guts of nodemon and all the help I can test on test coverage will be appreciated! |
@remy Seems the same issue still occurs for me with |
@julienvincent remind me what OS you're testing? Also can you try |
@remy That latest one ( |
okay, I'm going to push a PR, it'll kick off a load of tests, but hitting the sack now. I'll publish it in the morning if all the tests are good. I'm pretty sure this last version is the one. Thank you all (so far). |
Nice! Thanks for the good work 🙏 This thread was pretty entertaining. Until tomorrow! |
Fixes #1633 Multiple parts but all localised to run.js: - Refactor the kill logic to be simpler - Consistently use pstree to determine sub process list - Pipe stderr through nodemon to squash `sh -c` error warning Bug was caused by waiting on multiple sub processes and if they all ended the logic would only subtract one from the count list (rather than the total number). I've refactored the code so that it doesn't use the `kill -0 <pid>` as this was a little confusing to read (it's effectively a no op) and switched to using pstree to test if any sub processes are still running. The logic for killing the processes has also been refactored to simplify. Before it would fork the logic based on whether `ps` existed on the system. Now it uses the same logic with the exception of the kill signal sent - when `ps` isn't on the system, we have to send numeric signals (I can't remember how I found that out, but I do remember it was a painful process!). The last part required due to a side effect of the refactor on kill: when a kill signial is sent to `sh -c` the shell prints a warning. Details on how to replicate: https://git.io/Je6d0 To squash this, I track if the process is about to be killed (by flagging the sub process right before the kill function call) and if there's an error whilst shutdown is in effect, the error is only printed to nodemon's detailed output (using nodemon -V).
Confirming that |
Fixes #1633 Multiple parts but all localised to run.js: - Refactor the kill logic to be simpler - Consistently use pstree to determine sub process list - Pipe stderr through nodemon to squash `sh -c` error warning Bug was caused by waiting on multiple sub processes and if they all ended the logic would only subtract one from the count list (rather than the total number). I've refactored the code so that it doesn't use the `kill -0 <pid>` as this was a little confusing to read (it's effectively a no op) and switched to using pstree to test if any sub processes are still running. The logic for killing the processes has also been refactored to simplify. Before it would fork the logic based on whether `ps` existed on the system. Now it uses the same logic with the exception of the kill signal sent - when `ps` isn't on the system, we have to send numeric signals (I can't remember how I found that out, but I do remember it was a painful process!). The last part required due to a side effect of the refactor on kill: when a kill signial is sent to `sh -c` the shell prints a warning. Details on how to replicate: https://git.io/Je6d0 To squash this, I track if the process is about to be killed (by flagging the sub process right before the kill function call) and if there's an error whilst shutdown is in effect, the error is only printed to nodemon's detailed output (using nodemon -V).
Hi, On my docker container, I started to see the message
|
2.0.1 still have I use cluster |
@lorenc-tomasz can you raise a new issue with a simplified case using cluster. I've not got a test case for that usage so it will be useful to replicate with. |
Thanks @ezze :) |
nodemon -v
: 2.0.0node -v
: 12.13.1nodemon --exec 'npm run-script start'
Expected behaviour
Restarting a process with the
--exec
command cleanly terminates the existing processes and restarts the supplied command.Actual behaviour
When restarting a process, such as a node.js express server which has been started using the
--exec
option nodemon gets into an infinite loop, waiting for subprocesses to finish. Reportingcontinually in the console, interleaved with the normal messaging for restarting the process.
Steps to reproduce
I have created a basic repro repo here:
https://github.com/jayseeare/nodemon-bug-repro
Running
npm run debug
and making a change inindex.js
will exhibit the problem.Running
npm run watch
in the same repro does not exhibit the behaviour, and nor does running the same command with nodemon pinned tov1.19.1
.From the message and the versions I suspect this PR as having introduced the problem.
The text was updated successfully, but these errors were encountered: