Avoid panic on shut down when TLS configuration is present #8986
Conversation
Avoid panic when stopping gRPC Server if TLS configuration is present. Provided solution (attempts to) implement suggestion from gRPC team: grpc/grpc-go#1384 (comment). Fixes etcd-io#8916
embed/serve.go
Outdated
| } | ||
|
|
||
| // Gracefully shutdown gRPC server when using HTTP2 transport. | ||
| func stopInsecureServer(gs *grpc.Server, timeout time.Duration) { |
There was a problem hiding this comment.
This is very much redundant with https://github.com/coreos/etcd/blob/a2256a6f24115237f0046435d67e5548fe12744a/embed/etcd.go#L258
|
@jamesdphillips Approach looks good. Let me refactor this branch further, with additional tests around it. Thanks for contribution! |
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
| @@ -82,7 +82,7 @@ type Etcd struct { | |||
| type peerListener struct { | |||
| net.Listener | |||
| serve func() error | |||
| close func(context.Context) error | |||
| close func(time.Duration) error | |||
There was a problem hiding this comment.
not sure why do we need this change. can you explain?
There was a problem hiding this comment.
Previous (e *Etcd) stopGRPCServer(gs *grpc.Server) method was using embed.Etcd only to compute request timeout. Now, we want to use this same function with *http.Server parameter. Especially, in line https://github.com/coreos/etcd/pull/8986/files#diff-d7a47eb75475dba263540eb7f8456e50R258.
If we have time.Duration as an argument, it's easier to use this function for all shutdown calls.
There was a problem hiding this comment.
http server shutdown takes a context: https://golang.org/pkg/net/http/#Server.Shutdown
we should do the same, no?
|
Semaphore is not working in this branch for some reason https://semaphoreci.com/coreos/etcd/branches/pull-request-8986/builds/4. Moving to #8987. |
Fixes #8916
With regards to the panic gRPC team suggests:
Full comment can be found here.
Hope fix is satisfactory; happy to refactor if need be. 👍