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

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

Closed
odeadglaz opened this issue Jan 31, 2024 · 5 comments · Fixed by #464
Closed

Comments

@odeadglaz
Copy link
Contributor

odeadglaz commented Jan 31, 2024

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
Copy link
Contributor Author

Hey, I opened a draft PR for a potential fix here would love for any comments 🙏

@gustavohenke
Copy link
Member

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
Copy link
Contributor Author

Hey,

First thanks for the proposals, appreciated 👍

Both of these won't really do for me:

  • node-env won't just do, as I need to wait for successfull compilation of the main app or one if its workspace dependencies ( we are working with npm workspaces )
  • I don't wanna use restartTries as some commands that fail, should crash the process (unlike the restartable command)

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
Copy link
Member

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
Copy link
Contributor Author

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 ?

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

Successfully merging a pull request may close this issue.

2 participants