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
7.15k stars 233 forks source link

`killOthers` won't fully terminate others if also `restartTries >= 1` #496

Open sod opened 3 months ago

sod commented 3 months ago

If you use concurrently like:

node_modules/.bin/concurrently 'node foo.js 0' 'node foo.js 1' --kill-others --restart-tries 2
// foo.js - test script, `node foo.js 0` dies after 100ms, `node foo.js 1` stays alive
const id = process.argv[2];
console.log(`Hello world from id ${id}`);
if (id === '0') { setTimeout(() => { process.exit(1) }, 100) /* kill id 0 */ }
else { setTimeout(() => {}, 99999999) /* keep id 1 alive */ }

What happens:

It restarts process 0 twice, then runs the kill-others command. But the kill other is catched by another retry.

CleanShot 2024-08-05 at 14 20 10@2x

What I expected

if restart-tries has been exhausted, then kill-others should tear down the rest for good.

gustavohenke commented 3 months ago

Hey. This is how concurrently has always worked.

I think we can add a new flag/flag value to skip restarting other commands, if any of them has exhausted their restarts. Might be a bit counter intuitive, but I was thinking of something like --kill-others skip-restarts or --kill-others immediately - but I'm not very comfortable with either yet. Let me know if you got better ideas 😅

This should be reasonably easy to implement if you'd like to take a stab at it; I imagine it might be a simple takeUntil or takeWhile operator here:

https://github.com/open-cli-tools/concurrently/blob/23f97ac412f390a9913f96814da1f4070704d955/src/flow-control/restart-process.ts#L60-L61

sod commented 3 months ago

I'd file this under wrong behavior (as it kills and restarts the other one). But I fully understand that maintaining a popular library has to take into account that people now may rely on this behavior.

With that in mind, I'd implement the API like:

node_modules/.bin/concurrently \
    'node foo.js 0' \
    'node foo.js 1' \
    --kill-others \
    --total-restart-tries 2
#   ^^^^^^^^^^^^^^^^^^^^^^^^ a new setting

With:

--restart-tries 2 # default and old behavior - every command has its individual retry counter
--total-restart-tries 2 # new behavior - every command reduces a single counter - if that is exhausted, no further retries are being done

With this API, the new option also has a purpose on its own and isn't just there to workaround for --kill-others.