Skip to content

Conversation

@MaxMotovilov
Copy link

SIGTERM handler prevented process from ending - it has to stop the server explicitly in order for the rest of the cleanup logic to work as designed.

@coveralls
Copy link

coveralls commented Aug 30, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling f7011a3 on MaxMotovilov:master into 55c18ce on wbyoung:master.

@wbyoung
Copy link
Owner

wbyoung commented Aug 30, 2019

Ok, this looks good to me. Any docs that point to this being a requirement or is this just anecdotal? Can you find any docs for this requirement?

Also, I'm not actually using this personally any more, so to merge with confidence, I'd love it if we could find someone else to verify that this works and doesn't break any use cases.

Finally, in order to merge, the CI build will need to pass.

Thanks for contributing!

@MaxMotovilov
Copy link
Author

Please review; see http://man7.org/linux/man-pages/man7/signal.7.html for the official documentation on Unix signals.

If you do accept the PR, could you publish the package to npmjs? As things are now, the published version is already behind the master branch.

@wbyoung
Copy link
Owner

wbyoung commented Aug 30, 2019

Right, I know how signals work. I was curious about needing to stop the server. I'm surprised that this wouldn't have been something that I ran into originally. Any other instances you know of that are similar to this?

@MaxMotovilov
Copy link
Author

MaxMotovilov commented Aug 30, 2019

Stopping the server executes the listener in your code that removes all signal handlers and then event queue drains and node exits. Calling process.exit() from the signal handler works too (that's what I tried first) but it executes the code removing socket file 2nd time around and throws an error. Calling server.close() was simply the path of least resistance.

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

Successfully merging this pull request may close these issues.

3 participants