fix: wait for all child processes to terminate (fixes issue #1476) by marcelkottmann · Pull Request #1579 · remy/nodemon

marcelkottmann · 2019-06-07T14:47:22Z

remy · 2019-06-07T17:30:34Z

Is there a test for this that shows it failing and then passing with the change?

remy · 2019-06-07T17:32:51Z

Also, how is this different to the current kill logic?

marcelkottmann · 2019-06-07T18:13:34Z

Also, how is this different to the current kill logic?

The current kill logic is unchanged. After issuing the kill command in run.js:376 (which only sends signals to the other processes), the processes may need some time finishing.
The new command kill -0 actually does not send a signal, it only reports through the exit code, wether some given PIDs still exists, i.e. the process has not terminated. The new logic waits until every child process is really terminated. The current logic only waits for the parent child process (the sh command) to finish (the 'exit' event).

remy · 2019-06-07T19:03:49Z

And I assume this doesn't apply to windows? (I see Windows is being skipped)

marcelkottmann · 2019-06-07T19:56:12Z

And I assume this doesn't apply to windows? (I see Windows is being skipped)

As mentioned in my comment in #1476 I assume that the issue is only occuring on linux platform.

stale · 2019-06-21T20:51:59Z

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

Cerber-Ursi · 2019-06-26T06:59:26Z

@PePe79 Could you please look through the Travis error? I'm not sure if it is possible to fix it otherwise.

marcelkottmann · 2019-06-26T07:09:10Z

@Cerberuser yes I will have a look within the next 2 days.

marcelkottmann · 2019-06-27T12:40:02Z

Reworked this and moved the waitForOldKids function inside the kill function. I changed the behaviour in the way, that at first all kids are killed and only if all kids have been killed successfully (wait for it), killing also the parent process (the sh-process). This triggers the exit-event like before and restarts the monitored exec command.

dobesv · 2019-07-03T17:26:52Z

I think this may address the issue but it spams the console with messages "waiting for 1 child process to exit".

In our system we actually never exit when we receive the signal, we do a hot reload and ignore the signal. I think nodemon should allow this without spamming the console or using a lot of CPU cycles.

marcelkottmann · 2019-07-03T17:50:02Z

hey @dobesv , ok that sounds interesting. I did not thought about this use-case. It's conceptually the opposite of what is addressed in the issue #1476. I don't think that both use-cases

wait until the process has really terminated and then restart it

and

custom signalhandler without terminating the process

work together without a specific command line switch that tells nodemon what to do.

@remy What do you think about theses use-cases?

dobesv · 2019-07-03T18:14:34Z

lib/monitor/run.js

      } else {
        // make sure we kill from smallest to largest
-        const pids = kids.concat(child.pid).sort();
+        const pids = kids.sort();


You are mutating your parameter here, which is kind of a no-no. Perhaps copy the array first.

You're right. I fixed that by using .slice() now.

dobesv · 2019-07-03T18:31:39Z

lib/monitor/run.js

 var signals = require('./signals');

+function waitForOldKids(oldKids, callback) {
+  if (oldKids) {


Maybe also check oldKids.length > 0 ?

Ok, changed this line to if (Array.isArray(oldKids) && oldKids.length > 0) {

dobesv · 2019-07-03T18:32:57Z

lib/monitor/run.js

-        spawn('kill', ['-s', sig, child.pid].concat(kids))
-          .on('close', callback);
+        spawn('kill', ['-s', sig].concat(kids))
+          .on('close', waitForOldKids.bind(this, kids, function () {


If we filter out child.pid from kids here, we can just rely on the exit event from ChildProcess to detect the root process exit, which avoids the extra polling in the case that there's no extra child processes. e.g. kids.filter(p => p !== child.pid).

I think I know what you're meaning. But some commands won't create any sub processes. Maybe it is esoteric but for example nodemon --exec "read VAR" won't start a subprocess because read is a shell builtin command.

read VAR will start a shell subprocess, although there will be just one subprocess from nodemon's perspective. But in that case the exit event will be sent to us for that process so we don't need to poll for it to exit.

Or to clarify what I'm trying to say. nodemon --exec "read VAR" will at least start /bin/sh to run the read built-in. So there's still a child process there, but only one (/bin/sh). nodemon --exec "python foo.py" runs two subprocesses - /bin/sh and python.

Ok, you think kids contains also the sh process PID. I thought it doesn't (because of the previous old code in line 346: ..., child.pid].concat(kids)). I have to clarify what is right here.

Hmm actually I might have been confused about this one, I did a quick test and it seems like pstree is not including the parent id in the result.

e.g.

> child = require('child_process').exec('sleep 300s') ChildProcess { > child.pid 22955 > require('pstree.remy')(child.pid, (e, k) => console.log(k)) undefined > [ 22956 ]

I deleted some of my erroneous comments about this...

dobesv · 2019-07-03T18:34:40Z

The current release of nodemon supports both scenarios perfectly, as long as there is only one child process. I think a few simple changes will make your change more compatible with this:

Poll the children less often to avoid heavy CPU use (once per second is probably fine)
Don't poll the child.pid, since node will notify us of its exit via the event system. If there are no extra child processes the polling won't run at all, and your change won't affect users who don't need it.
Don't log on every poll; maybe just log if the number of remaining processes changed from the last time (or don't log at all)

dobesv · 2019-07-03T18:40:56Z

lib/monitor/run.js

+    exec('kill -0  ' + oldKids.join(' '), (error) => {
+      const returnCode = error ? error.code : 0;
+      if (returnCode < 126) { // ignore command not found error
+        const stillRunningKids = oldKids.length - returnCode;


When I run kill it always returns 0 or 1 so I'm not sure if this math is reliable for cases where there are multiple child processes. You might have to run psTree again for each child to see whether it is still running and whether it has added any new child processes.

Maybe what you could do is just poll kids[0]. If it isn't running any more, call waitForOldKids(oldKids.slice(1), callback). Then when oldKids.length === 0 all the kids are done.

Instead of using kill to poll you could use psTree. It'll return an empty list if the process has exited, but if the list is not empty you can union that with the list you have so that you can detect any new child processes that may have been spawned.

Also, using psTree to wait for the child process might work in windows as well, where kill is not available.

Actually since psTree doesn't return the pid given as part of the list, you can't use it to tell whether a process is still running.

In this part of code I only want to wait for the subprocesses to terminate. I think using pstree here instead of kill -0 is a good idea. I will try that out.

Oh yeah, if you pass in the root pid you can just us pstree to check if there are any children or not instead of trying to kill each child.

dobesv · 2019-07-03T18:41:50Z

lib/monitor/run.js

+        if (stillRunningKids > 0) {
+          utils.log.status('still waiting for ' + stillRunningKids +
+            ' child process(es) to finish...');
+          setTimeout(waitForOldKids.bind(this, oldKids, callback), 100);


A slower polling interval might be nice for processes that aren't actually planning to exit.

marcelkottmann · 2019-07-03T19:16:34Z

The current release of nodemon supports both scenarios perfectly, as long as there is only one child process.

I think your assumption is wrong, because nodemon itself starts a sh process, which then starts the provided exec command. So the "normal" usage of nodemon with launching one simple command, launches at least two processes. The exit-handler of nodemon only handles the exit of the sh command which does not gurantee that the subprocess (the exec command) also has terminated (at least on linux).

dobesv · 2019-07-03T19:24:25Z

The current release of nodemon supports both scenarios perfectly, as long as there is only one child process.

I think your assumption is wrong, because nodemon itself starts a sh process

If you run nodemon bla.js it doesn't run sh, it just forks a new node process and passes the script to it directly. In that case the bug(s) you are trying to fix here do not manifest because there is only a single child process.

Currently these bugs don't affect me because I only ran into them while trying pass arguments to node when running with nodemon. This triggered it to use sh as an intermediate process to pass the arguments. To resolve this I now pass arguments to node via environment variables instead of arguments through nodemon. Thus the issues at hand do not currently affect my project.

Your fix, were it to be merged, might affect me if it changes the behavior of the case that currently works correctly for me (single child process).

stale · 2019-08-21T16:19:35Z

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

Cerber-Ursi · 2019-08-21T17:25:04Z

@PePe79, is there any progress, or the PR is really stale?

marcelkottmann · 2019-08-23T05:50:29Z

No progress from my side, because of lack of time. Next steps will be looking at some of the findings of @dobesv and thinking about how to create a testcase that shows the errorcase. Unfortunately there will be no progress from my side in the next few weeks.

stale · 2019-09-06T06:40:50Z

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

Cerber-Ursi · 2019-09-06T06:43:35Z

Hope this issue isn't stale in fact.

stale · 2019-09-20T06:56:32Z

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

orinokai · 2019-09-20T07:58:45Z

Eagerly awaiting this PR, thanks for everyone's efforts so far. Definitely not stale, issue persists.

Remzi1993 · 2019-10-21T12:21:38Z

I'm also waiting for this. @remy Could you check if all is okay?
It seems like it passed all checks.

remy · 2019-10-24T08:11:16Z

@Remzi1993 & @PePe79 I'm blocking out time these next few days for nodemon bits - I had one request on this code (I'll add as review comment too), can we rename kids to subprocess (I have infant mortality in my personal life and I'd like to keep it out of my code - there's no reason you might know, but if we can rename, then I'll start a detailed review and test and should get it merged and live this week).

Remzi1993 · 2019-10-24T08:18:06Z

Hi @remy
I'm so sorry to hear this. Maybe I can help renaming everything, I'm also for logical names and I also avoid in my coding analogies like this to avoid confusing what it actually might mean.

I will take a look at the code this sunday (but I can't promise anything, because I'm very busy with a course/academy till the end of November).

Kind regards,
@Remzi1993

remy · 2019-10-24T10:05:51Z

Thanks for understanding, and cheers.

…

On Thu, 24 Oct 2019, 09:18 Remzi Cavdar, ***@***.***> wrote: Hi @remy <https://github.com/remy> I'm so sorry to hear this. Maybe I can help renaming everything, I'm also for logical names and I also avoid in my coding analogies like this to avoid confusing what it actually might mean. I will take a look at the code this sunday (but I can't promise anything, because I'm very busy with a course/academy till the end of November). Kind regards, @Remzi1993 <https://github.com/Remzi1993> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1579?email_source=notifications&email_token=AAADLBHY7XRMEY6JZVBJ7FDQQFK4BA5CNFSM4HVXYBG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECEEXEA#issuecomment-545803152>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADLBDXIN56T6SBBYGOUA3QQFK4BANCNFSM4HVXYBGQ> .

marcelkottmann · 2019-10-24T10:58:33Z

Hi @remy , I am sorry for you. That's totally understandable and renaming shouldn't be a problem. Sorry that I did not have the time to create a testcase that can be automated. I had some ideas to test this inside docker containers, but hadn't any time to try that...
I will rename the variables and push the changes within the next two hours.

marcelkottmann · 2019-10-24T12:03:33Z

I also created a git repo with a testcase to reproduce this issue and to test this fix (see README): https://github.com/pepe79/test-nodemon-issue-1476

remy · 2019-10-30T22:33:31Z

Picking this up this week - thank you @PePe79 for the changes.

remy · 2019-11-20T15:18:19Z

Merged and going live in PR #1632

remy · 2019-11-21T10:22:15Z

I'm in the process or looking at #1633 in detail because this change isn't working as expected in Ubuntu with a standard setup.

I think the problem is when there's more than one sub process, the kill -0 1234 6789 runs and it gives an error of code: 1 indicating that one or more of the processes are stopped, assuming both have stopped, the maths is then wrong, subProcesses.length - returnCode # 2 - 1.

The logic should either run this function for each sub process or it should revalidate whether the processes are running with pstree. I need to look back through the comments here as to why pstree wasn't used though.

marcelkottmann · 2019-11-22T15:31:22Z

@remy , sorry didn't see that coming. On my system kill -0 returns exit code 2 if both given pids terminated...

> kill -0 4567 6789
kill: kill 4567 failed: no such process
kill: kill 6789 failed: no such process
> echo $?
2

Using pstree is the much better cross-platform and portable way. Sorry for the inconvience.

marcelkottmann mentioned this pull request Jun 7, 2019

Nodemon Fails to Wait for Process Exit when using Docker + Inspector #1476

Closed

marcelkottmann force-pushed the master branch 2 times, most recently from 9e7a2e2 to f157443 Compare June 7, 2019 15:14

stale bot added stale no activity for 2 weeks and removed stale no activity for 2 weeks labels Jun 21, 2019

marcelkottmann force-pushed the master branch from f157443 to 6abc2e4 Compare June 27, 2019 12:34

marcelkottmann changed the title ~~fix: wait for _all_ child processes to terminate (fixes issue #1476)~~ fix: wait for all child processes to terminate (fixes issue #1476) Jun 28, 2019

marcelkottmann mentioned this pull request Jul 3, 2019

Nodemon does not wait for process to exit before starting a new one #1508

Closed

dobesv reviewed Jul 3, 2019

View reviewed changes

marcelkottmann force-pushed the master branch from 6abc2e4 to adc7c5d Compare July 3, 2019 18:30

dobesv reviewed Jul 3, 2019

View reviewed changes

marcelkottmann force-pushed the master branch 2 times, most recently from 760d1d9 to f6c2841 Compare July 3, 2019 18:49

stale bot added the stale no activity for 2 weeks label Aug 21, 2019

stale bot removed the stale no activity for 2 weeks label Aug 21, 2019

stale bot added the stale no activity for 2 weeks label Sep 20, 2019

stale bot removed the stale no activity for 2 weeks label Sep 20, 2019

remy added not-stale Tell stalebot to ignore this issue waiting for changes labels Sep 28, 2019

marcelkottmann force-pushed the master branch from f6c2841 to 9a951fa Compare October 24, 2019 11:58

fix: wait for all subprocesses to terminate (fixes issue remy#1476)

90fc149

marcelkottmann force-pushed the master branch from 9a951fa to 90fc149 Compare October 24, 2019 12:15

This was referenced Nov 20, 2019

Error: listen EADDRINUSE #1629

Closed

Nodemon not completing restart of server on change of server side code #1630

Closed

remy closed this Nov 20, 2019

jaybeeuu mentioned this pull request Nov 20, 2019

Nodemon waits forever for subprocesses to end #1633

Closed

Uh oh!

Conversation

marcelkottmann commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remy commented Jun 7, 2019

Uh oh!

remy commented Jun 7, 2019

Uh oh!

marcelkottmann commented Jun 7, 2019

Uh oh!

remy commented Jun 7, 2019

Uh oh!

marcelkottmann commented Jun 7, 2019

Uh oh!

stale bot commented Jun 21, 2019

Uh oh!

Cerber-Ursi commented Jun 26, 2019

Uh oh!

marcelkottmann commented Jun 26, 2019

Uh oh!

marcelkottmann commented Jun 27, 2019

Uh oh!

dobesv commented Jul 3, 2019

Uh oh!

marcelkottmann commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobesv commented Jul 3, 2019

Uh oh!

dobesv Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobesv Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcelkottmann Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcelkottmann commented Jul 3, 2019

Uh oh!

dobesv commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Aug 21, 2019

Uh oh!

Cerber-Ursi commented Aug 21, 2019

marcelkottmann commented Jun 7, 2019 •

edited

Loading

marcelkottmann commented Jul 3, 2019 •

edited

Loading

dobesv Jul 3, 2019 •

edited

Loading

dobesv Jul 3, 2019 •

edited

Loading

marcelkottmann Jul 4, 2019 •

edited

Loading

dobesv commented Jul 3, 2019 •

edited

Loading