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

[Bug Report]: CTRL+c does not kill the process #181

Open
IlyaRazuvaev opened this issue Apr 20, 2023 · 1 comment · May be fixed by #182
Open

[Bug Report]: CTRL+c does not kill the process #181

IlyaRazuvaev opened this issue Apr 20, 2023 · 1 comment · May be fixed by #182
Labels
T: bug Functionality that does not work as intended/expected

Comments

@IlyaRazuvaev
Copy link

Environment
Carbone Version: 3.5.5
Node Version: 18.13.0
Desktop OS: tested on "darwin" (Mac), "win32" (Windows 10).

Related issue:
#179 (comment)

CTRL+c does not kill the process in new version of carbone.

@IlyaRazuvaev IlyaRazuvaev added the T: bug Functionality that does not work as intended/expected label Apr 20, 2023
@genadis
Copy link

genadis commented Apr 21, 2023

The issue is due to this addition: https://github.com/carboneio/carbone/blob/master/lib/converter.js#L659 :

['SIGINT', 'SIGHUP', 'SIGQUIT'].forEach(function (signal) {
  process.on(signal, function () {
    converter.exit();
  });
});

According to Nodejs docs, adding listener to those events override default OS behaviour causing the process to hang and never exit:
https://nodejs.org/docs/latest-v18.x/api/process.html#signal-events

changing the code to below fixes the issue, and does not break any custom handling for apps that use carbone and require their own custom handling of the signals:

['SIGINT', 'SIGHUP', 'SIGQUIT'].forEach(function (signal) {
    const cleanup = () => {
        converter.exit(() => {
            process.removeListener(signal, cleanup);
            process.kill(process.pid, signal);
        });
    }
    process.on(signal, cleanup);
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants