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

embed: fix *grpc.Server panic on GracefulStop with TLS-enabled server #8987

Merged
merged 3 commits into from
Dec 8, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Dec 7, 2017

Replace #8986.
Fix #8916.

@gyuho
Copy link
Contributor Author

gyuho commented Dec 7, 2017

@jamesdphillips @xiang90 Semaphore CI in previous PR was not working for some reason. Creating a fresh branch for proper testing.

embed/etcd.go Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://golang.org/pkg/net/http/#Server.Shutdown takes context. i am not sure why we want to change to time.duration.

@@ -219,23 +219,30 @@ func (e *Etcd) Config() Config {
return e.cfg
}

// Close gracefully shuts down all servers/listeners.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should mention there is a default timeout for the graceful shutdown. or we will do a force shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiang90 Comments are added. PTAL.

embed/etcd.go Outdated
shutdownNow := func() {
// first, close the http.Server
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ss.http.Shutdown(ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiang90 Here we construct the context for Shutdown?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not pass in ctx direction? i just not sure why we want duration at the first place.

embed/etcd.go Outdated
timeout := 2 * time.Second
if e.Server != nil {
timeout = e.Server.Cfg.ReqTimeout()
func stopServers(ss *servers, timeout time.Duration) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not take a context but time.duration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that works too. Let me change to context.

@gyuho gyuho force-pushed the tls-shutdown branch 3 times, most recently from 62aa2e3 to efe2c19 Compare December 7, 2017 20:40
@gyuho gyuho changed the title embed: do not call GracefulShutdown on insecure gRPC server embed: do not call GracefulStop on insecure gRPC server Dec 7, 2017
@gyuho gyuho changed the title embed: do not call GracefulStop on insecure gRPC server embed: do not call GracefulStop on secure gRPC server Dec 7, 2017
@gyuho gyuho changed the title embed: do not call GracefulStop on secure gRPC server embed: fix *grpc.Server panic on GracefulStop with TLS-enabled server Dec 7, 2017
@gyuho gyuho changed the title embed: fix *grpc.Server panic on GracefulStop with TLS-enabled server embed: fix *grpc.Server panic on GracefulStop with TLS-enabled server Dec 7, 2017
@xiang90
Copy link
Contributor

xiang90 commented Dec 8, 2017

lgtm if CIs all turn green.

@gyuho
Copy link
Contributor Author

gyuho commented Dec 8, 2017

GitHub CI status polling seems outdated. All CIs passed (https://jenkins-etcd.prod.coreos.systems/job/etcd-proxy/3639/). Will merge tomorrow.

jamesdphillips and others added 3 commits December 7, 2017 20:36
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
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@fc2eecf). Click here to learn what that means.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8987   +/-   ##
=========================================
  Coverage          ?   76.03%           
=========================================
  Files             ?      359           
  Lines             ?    29819           
  Branches          ?        0           
=========================================
  Hits              ?    22672           
  Misses            ?     5563           
  Partials          ?     1584
Impacted Files Coverage Δ
embed/serve.go 68.21% <100%> (ø)
embed/etcd.go 69.28% <86.95%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc2eecf...9bd07c9. Read the comment docs.

@gyuho gyuho merged commit 015c04b into etcd-io:master Dec 8, 2017
@gyuho gyuho deleted the tls-shutdown branch December 8, 2017 14:40
@jamesdphillips
Copy link
Contributor

Late response but thank you!

gyuho added a commit that referenced this pull request Jan 2, 2018
gyuho added a commit that referenced this pull request Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants