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

Gracefully shutdown #260

Closed
StarpTech opened this issue Sep 19, 2017 · 14 comments
Closed

Gracefully shutdown #260

StarpTech opened this issue Sep 19, 2017 · 14 comments
Labels
plugin suggestion Ideas for new plugins

Comments

@StarpTech
Copy link
Member

The server should shutdown gracefully that implicit

  • Handle SIGINT, SIGTERM
process.on('SIGTERM', function onSigterm () {  
  // fire onClose hook
  // close server
})
@mcollina
Copy link
Member

I’m really -1 to have them as default features. They seems very good ideas on the application side, but they cause loads of interactions with other modules, as process is a global.
I have aready been there.

IMHO it’s perfect content for a plugin.

@StarpTech
Copy link
Member Author

Do you mean a plugin should emit the onClose hook and gracefully shutdown fastify?

@mcollina
Copy link
Member

You just need to add the logic to gracefully shutdown in the plugin, and call close  when 'SIGTERM' is called.

@StarpTech
Copy link
Member Author

Add label "Plugin suggestion" ? :D

@mcollina mcollina added the plugin suggestion Ideas for new plugins label Sep 20, 2017
@cagataycali
Copy link
Contributor

I think this should be done by the developer. How much do you need to plug in for this? What kind of usage do you think

@StarpTech
Copy link
Member Author

Hi, could you check it out https://github.com/hemerajs/fastify-graceful-shutdown

@jsumners
Copy link
Member

You really don't want to listen to SIGTERM and SIGINT from a library. You'll end up with code like:

And having to make very clear to users that you have registered for those events. Once something is registered on those events Node does not perform its normal shutdown procedure.

@StarpTech
Copy link
Member Author

What is the alternative to gracefully shutdown the application? You have to listen on events outside of your environment e.g in container environment or through a simple ctrl+c.

@StarpTech
Copy link
Member Author

I agree with you it can complicate things but at first, this functionality is provided as a plugin and if you know that you have one interface to release your resources, it's legitimate.

@StarpTech
Copy link
Member Author

StarpTech commented Sep 24, 2017

Pino is always a part of a bigger application that's why you have to care about your environment. Fastify is the head of your application and if you want to release something use fastify.gracefulShutdown

@mcollina
Copy link
Member

IMHO listening to those events in a library is always going to cause problems, as it is assuming that a framework is going to “control” the process. There are a lot of scenarios where that causes very hard to debug scenarios. IMHO it is not a functionality I’m ok to maintain, however if you want to release it as a plugin, it should contain a big warning that it should not be registered twice, or the process might be left in an hard to debug state.

I’ll do a quick review of the code, and maybe suggest a few counterbalances to the most common problems.

@jsumners
Copy link
Member

I just don't see the point. All of this is stuff I would put in a processManagement.js of my application, e.g. https://github.com/jscas/cas-server/blob/master/lib/processManagement.js. Applications should be managing the resources they use and libraries should be invisible.

@StarpTech
Copy link
Member Author

StarpTech commented Sep 25, 2017

@jsumners I'm on your side but this plugin should just support it and centralize the functionality. If you create a plugin which has to clean up resources you can register it with fastify.gracefulShutdown I don't see the issue in this. You would create the same functionality in your plugins more than one time. This way is much simpler and maintainable.

The main difference is that we are talking about a plugin system not applications. In this case fastify should provide the interface (e.g by plugin)

@mcollina
Copy link
Member

I don't see this discussing going anywhere. The functionality is already done inside a plugin and we can close this.

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

No branches or pull requests

4 participants