Use context in Server #3007
Use context in Server #3007
Conversation
| ) | ||
|
|
||
| func (s *Server) configureSignals() { | ||
| signal.Notify(s.signals, syscall.SIGINT, syscall.SIGTERM) |
server/server.go
Outdated
| go func() { | ||
| defer s.Close() | ||
| <-ctx.Done() | ||
| log.Infof("I have to go...") |
There was a problem hiding this comment.
Could you please use a log.Info instead of log.Infof
cmd/traefik/traefik.go
Outdated
| @@ -173,7 +174,8 @@ func runCmd(globalConfiguration *configuration.GlobalConfiguration, configFile s | |||
| acme.Get().SetConfigListenerChan(make(chan types.Configuration)) | |||
| svr.AddListener(acme.Get().ListenConfiguration) | |||
| } | |||
| svr.Start() | |||
| ctx := server.ContextWithSignal(context.Background()) | |||
There was a problem hiding this comment.
should this be just in the main (or in cmd/traefik in that case). The server package doesn't really have to care about signal handling.
There was a problem hiding this comment.
I'm ok with this, I will move ContextWithSignal
server/server.go
Outdated
| @@ -235,14 +252,13 @@ func (s *Server) Stop() { | |||
|
|
|||
| // Close destroys the server | |||
| func (s *Server) Close() { | |||
| ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.globalConfiguration.LifeCycle.GraceTimeOut)) | |||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) | |||
There was a problem hiding this comment.
Why the magic time number here instead of using GraceTimeOut ?
There was a problem hiding this comment.
In fact, this timeout is for the stop of goroutines in Providers and Metrics, not for GraceTimeOut on entry points.
Moreover it's a technical timeout, there is no reason to configure it.
There was a problem hiding this comment.
Are we certain that those 10 seconds are always enough for providers to finish?
There was a problem hiding this comment.
In reality, I reuse the previous DefaultValue of GraceTimeOut. So, if you don't override the GraceTimeout, it will not really change the behavior. Moreover, as I change the os.Exit to panic, I think we will have more feedback if some goroutines never end or needs a bigger timeout (without change the "real" behavior, because os.Exit or panic will only change the "logs".
There was a problem hiding this comment.
Where is GraceTimeOut being used now? I see that the PR removes an existing usage but does not add it anywhere again.
There was a problem hiding this comment.
GraceTimeOut is used when we close entrypoints. https://github.com/containous/traefik/blob/526a04d4c8c0ad042c69c3ec7c48ab373e97627b/server/server.go#L221
traefiker
force-pushed
the
server-context
branch
from
4007c1c
to
71d5b47
Compare
What does this PR do?
Use
contextwhen the server Start and then,cancelthecontextwhen we receive signals ( SIGINT, SIGTERM ).Close timeout is no longer
RequestAcceptGraceTimeoutand if the Close timeout, wepanicinstead of os.ExitMotivation
Separation of concern
Additional Notes
Close timeout is a timeout to wait for stop of goroutines (provider and metrics). So it make no sense to configure this timeout. Moreover if we failed to stop goroutines, it's a real problem and os.Exit is not enough verbose.