Added option to disable signal handling by Server.#191
Added option to disable signal handling by Server.#191bboreham merged 6 commits intoweaveworks:masterfrom
Conversation
annanay25
left a comment
annanay25
left a comment
There was a problem hiding this comment.
Thanks for this change, @pstibrany! I agree with the general direction.
|
Any chance of getting this merged? We would like to know whether we can simplify the design of Cortex modules proposal (if this is accepted), or not. Thank you very much! (If any changes are needed, please let me know) |
server/server.go
Outdated
| }() | ||
| } else { | ||
| go func() { | ||
| <-s.quit |
There was a problem hiding this comment.
Seems like we have many ways to quit now and I don't know why.
Why do we need Stop vs Shutdown?
There was a problem hiding this comment.
Stop vs Shutdown was there before. Run is a blocking method, that returns first error from HTTP/gRPC or nil, if signal has been received or Stop was called.
Shutdown stops gRPC and HTTP servers.
Since Stop works by stopping signal handler, which can no longer be there, I've kept the functionality of Stop by using quit channel.
If we were doing design from scratch, we could replace blocking Run method with non-blocking Start (perhaps returning channel with errors), which would make Stop unnecessary.
There was a problem hiding this comment.
Looking further, the signal-handler has another Stop() mechanism SignalReceiver, which appears to be impossible to use from Server.
How about making handler something you can pass in config, like Log, that has interface Loop() and Stop(), then Cortex can pass either what you're doing with quit or its own actual signal handler?
There was a problem hiding this comment.
That sounds good, I'll update PR with your suggestion.
|
I've updated PR with interface as per suggestion. I've also modified unrelated |
|
Thanks! |
…works#191) * Implement common file server to return consistent Content-Type When `/etc/mime.types` has a unusual mime type, Content-Type of response becomes the type and you may get unexpected result. This server returns consistent Content-Type header for js and css files Signed-off-by: mrasu <m.rasu.hitsuji@gmail.com>
3 participants
In Cortex, we currently have extra logic (as explained here) to handle the fact that
server.Servertype handles SIGINT/SIGTERM signals, that should trigger shutdown of entire server.This PR adds ability to disable signal handling by Server. This will make it possible to handle signal by Cortex
Runmethod instead, and to trigger shutdown of all services (incl.Server) cleanly, without doing special hacks for "Server" service.Option to disable signal handling is deliberately "negative" ("disable", not "enable"), to keep existing code working as before.