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

Abort commands not running when max processes < N #460

Merged
merged 11 commits into from
Mar 24, 2024

Conversation

gustavohenke
Copy link
Member

@gustavohenke gustavohenke commented Jan 10, 2024

Problem

In a few issues, this situation came up where commands that haven't started yet will run anyways when they shouldn't even start.
Best examples of this are SIGINT and --kill-others combined with --max-processes that's lower than N (where N = number of commands itself).

For someone who pressed Ctrl + C, it could be annoying that one press isn't sufficient and they have to press it up to N times (see #452).

For --kill-others/--kill-others-on-fail, it, well... just doesn't work (see #433).

Solution

With SIGINT, it's quite clear that commands awaiting their turn should be aborted.

For --kill-others, there was a possible interaction with --restart-tries: should it be respected at all?
But the flag is documented as "restart processes that died", which isn't the case of a command that never started. Therefore I made the decision of also aborting these commands.

Now, aborting the commands is done here by using an AbortController.
It's quite simple: flow controllers make use of it, and concurrently stops further spawning of processes if the signal is already aborted.

Note

AbortController is built into Node 15+, which is a requirement for concurrently 9.0.0 (#448), therefore this PR is a breaking change.


Fixes #433
Fixes #452

@gustavohenke gustavohenke added this to the v9 milestone Jan 10, 2024
@coveralls
Copy link

coveralls commented Jan 10, 2024

Coverage Status

coverage: 99.346% (+0.02%) from 99.324%
when pulling 91d6b3f on issues-433-and-452
into bae6fe5 on main.

@gustavohenke gustavohenke marked this pull request as ready for review January 10, 2024 16:34
@gustavohenke
Copy link
Member Author

@paescuj keen to take a look?

@gustavohenke gustavohenke merged commit 8ab6ff1 into main Mar 24, 2024
21 checks passed
@gustavohenke gustavohenke deleted the issues-433-and-452 branch March 24, 2024 21:43
@gustavohenke
Copy link
Member Author

🚢 This is now fixed in v9.0.0!
https://github.com/open-cli-tools/concurrently/releases/tag/v9.0.0

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