open-cli-tools / concurrently

Run commands concurrently. Like `npm run watch-js & npm run watch-less` but better.
https://www.npmjs.com/package/concurrently
MIT License
7k stars 228 forks source link

`--kill-others-on-fail` flag not working as expected with `--group --max-processes 1` #433

Closed daniil4udo closed 5 months ago

daniil4udo commented 1 year ago

Description

When trying to run multiple scripts sequentially using concurrently, I noticed that the --kill-others-on-fail flag is not behaving as expected when used together with the --group --max-processes 1 flags, rest of the scripts still running.

Expected Behavior

If one script fails (exits with a non-zero code), all running processes should be terminated when using the --kill-others-on-fail flag, even when running scripts sequentially with --group --max-processes 1.

Current Behavior

However, when I use the --kill-others-on-fail flag in conjunction with --group --max-processes 1, concurrently is not terminating all running processes when one script fails. This behaviour is not observed when I remove --group --max-processes 1 from the command.

gustavohenke commented 1 year ago

Thanks for the report! I think this use case makes sense.

There's a potentially funny interaction with --restart-tries here. The first command will restart N times before giving up, but should other commands be given a chance to spawn at all?

bozdoz commented 9 months ago

How about a --kill-others-on-sigint? I'm not sure I understand the interaction with "restart" here. If the user cancels the process, I'd expect the main process (concurrently) to stop the subprocesses.

Don't the restart flags default to 0?

gustavohenke commented 9 months ago

Don't the restart flags default to 0?

Yes.

The issue I see starts like this:

concurrently --kill-others-on-fail --max-processes 1 "sleep 5 && echo foo" "exit 1"

But once you add --restart-tries 1, then it's fair that command 0 gets a chance to run... or not really?

bozdoz commented 9 months ago

Should restart give the commands a chance to run? No. Not on SIGINT. The user is aborting the process.

Thinking of docker restart policies:

None of these restart when the container is manually stopped: even "always" explicitly doesn't include if it's manually stopped.

gustavohenke commented 9 months ago

Yeah, SIGINT has a quite clear use case.

So, --restart-tries is documented as "restart processes that died". If there's a limited concurrency, then not all commands will be running at once, so they should probably not be given a chance to spawn at all when --kill-others/--kill-others-on-fail is set.

I think this makes sense. WDYT?

gustavohenke commented 1 week ago

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