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

node app not killed by killer.js #71

Closed
zontarian opened this issue Sep 12, 2019 · 3 comments
Closed

node app not killed by killer.js #71

zontarian opened this issue Sep 12, 2019 · 3 comments

Comments

@zontarian
Copy link

Hello,
we are developing a nest app (inside a docker container). Everything works fine with tsc-watch but when code is being changed, dist is being rebuilt, the node process is not restarted.

The relevant code is package.json is

"scripts": {
    "start": "ts-node -r tsconfig-paths/register src/main.ts",
    "start:dev": "tsc-watch -p tsconfig.build.json --onSuccess 'node dist/main.js'",
  },

we are using start:dev.

We have tracked down the problem to the killer.js source file issueing a SIGUSR2 signal to node. Our basic node app does not handle this signal so it just ignores the signal, and does not die.

Maybe it's because nest handles SIGUSR2 in its own way. We don't know.

We have solved the issue in two ways:

  1. changing killer.js (killer.js) to have let KILL_SIGNAL = 'SIGTERM';
  2. adding a process.on('SIGUSR2', () => { process.exit();}); to our main.ts app file.

Either way makes the tsc-watch work as expected

But we are wondering why you put SIGUSR instead of SIGTERM, or what are we missing, since nobody seems to have our problem.
Could you help us better understand the basic issue here? Thanks.

Anyway thanks for your work for the node community,
regards

@gilamran
Copy link
Owner

Hi @zontarian
As far as I unserstand SIGUSR2 is a signal that let's the process terminate itself, it lets the process clean up if needed. SIGTERM will terminate the process without giving it the time to clean up. Correct me if I'm wrong.

Also, note that (according to nodejs docs) windows does not support SIGTERM.

We can change this behavior, and ask the process to terminate using SIGUSR2 and with timeout try to terminate it using SIGTERM.

I don't know how nest handles all these signals, but you can try to start your nest app, and kill it using kill -SIGUSR2 $pid or kill -SIGTERM $pid.

@zontarian
Copy link
Author

zontarian commented Sep 13, 2019

Hi @gilamran ,

setting aside Windows, which of course does not handle signals in general (let alone SIGUSR2), and sticking to unix/mac, SIGTERM is a signal that can be trapped by any app and used to do clean up routines (as far as I remember from college). SIGKILL is the one that cannot be trapped and is equivalent to the kill -9 $pid used when programs are stuck or keeps respawning children. It's the equivalent of "force quit" a program in Mac for example.
From unix manuals SIGUSR* are open signal left to program developers to use if they want, but which are ignored by te OS.

SIGUSR2indeed seems to terminate for example bash scripts and the pure node executable, but not nest. Before trying to modify your code we did try to send SIGUSR2 to our nest app. The app still ran. sending SIGTERM (which is equivalent to kill $pid) did the trick.

I think that you can implement two solutions:

  1. the one you suggest: set a timeout and then try SIGTERM (this could somehow slow down rapid development though, depending on timeout length :)
  2. add an option on the command line -terminate-signal SIGNAL that let the user specify the signal used to kill the program
  3. specify in the docs that the signal sent is SIGUSR2 and suggest users to check that their programs is indeed termianted upon receiving it, or suggesting a simple modification for nodeJS app to add an exit handler, if their node app does not respond to it (the trick is the one we have used)
    but of course this is up to you.

Anyway at the moment we stick with solution 3 since it does not change your code.

Thanks for your reply
Z

@gilamran
Copy link
Owner

Fixed and published a major version.
npm i tsc-watch@latest To install the latest

Thanks for all the help!

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

No branches or pull requests

2 participants