fix: ubuntu loop waiting for sub processes #1635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1633
Multiple parts but all localised to run.js:
sh -cerror warningBug 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>asthis 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
psexistedon the system.
Now it uses the same logic with the exception of the kill signal sent -
when
psisn't on the system, we have to send numeric signals (I can'tremember 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 -cthe 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).