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
6.98k stars 227 forks source link

Programmatically restarting Command using `kill/start` API produce wrong termination #463

Closed odeadglaz closed 5 months ago

odeadglaz commented 7 months ago

Description I'm using concurrently to run 2 commands in parallel on my application:

  1. Watch's application code using swc
  2. A plain node ./dist/server.js command which starts my HTTP server.

I wanted to implement a logic of which every time SWC outputs a successful watch compilation output, to restart the NodeJS server by combining the Command object kill/start API.

Here is a simplified example of my code:

const commands = [
    { name: 'SWC watcher', command: 'swc src -d dist/server -w -D' },
    { name: 'HTTP Server', command: 'node -r dotenv/config ./dist/server/index.js' },
];

const concurrency = concurrently(commands, {
    killOthers: ['failure'],
    prefixColors: ['magenta', 'yellow']
});

let shouldRestart = false;
const [watcherCommand, serverCommand] = concurrency.commands;

serverCommand.close.subscribe(() => {
    if (shouldRestart) {
        shouldRestart = false;
        serverCommand.start();
    }
});

watcherCommand.stdout.subscribe((data) => {
    const input = data.toString().trim();

    if (compilationCompleteRegex.test(input)) { // Detects compilation end output
        shouldRestart = true;
        serverCommand.kill('SIGTERM'); // HTTP listens and gracefully shuts down
    }
});

Actual Behaviour After 2 restarts the concurrency top command resolves as both commands was terminated but in fact, both commands still up and running.

Some Technical details It seems that CompletionListener.listen method actually uses bufferCount to assume its commands were terminated by counting the close events but programmatically using Command.start/Command.kill API's can cause un-expected behaviour in this manner as nothing was actually terminated rather solely restarted

Thanks in advanced šŸ™

odeadglaz commented 7 months ago

Hey, I opened a draft PR for a potential fix here would love for any comments šŸ™

gustavohenke commented 7 months ago

I recommend using something like node-dev.

If you don't want that, or it doesn't work for some reason, another solution you could use today is to set restartTries: -1. This makes killed processes restart forever, so you don't need to manually restart. Completion is autmoatically handled in this case.

But for restarting at a random moment, there'll will still be issues, as you demonstrate.

odeadglaz commented 7 months ago

Hey,

First thanks for the proposals, appreciated šŸ‘

Both of these won't really do for me:

I think i will create a process, that just stays up for ever and manage a subprocess with restarts undertneeth, where as far as concurrently all commands will ever run if we won't have a near term solution

gustavohenke commented 7 months ago

I don't wanna use restartTries as some commands that fail, should crash the process (unlike the restartable command)

If your app doesn't need to gracefully exit, you should be able to send a SIGKILL instead of a SIGTERM in this case

odeadglaz commented 7 months ago

It doesn't as we are using this for local development mode, tho I'm not sure how using SIGKILL will help here? Won't it emit close event of that command as-well ?