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: stop *grpc.Server on *serveCtx serve error #8992

Merged
merged 1 commit into from
Dec 9, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Dec 8, 2017

I forgot to handle this in #8987.

If serve errors before *grpc.Server is sent to serversC,
it should be closed manually.

@xiang90

@xiang90
Copy link
Contributor

xiang90 commented Dec 8, 2017

can we add a test case to cover this?

@gyuho
Copy link
Contributor Author

gyuho commented Dec 8, 2017

@xiang90 It's hard unless we refactor embed package to break it down to smaller pieces.

In integration tests, I tried triggering failure to this path by passing nil *transport.TLSInfo to serve, but it is being passed by value, so impossible to trigger error outside of embed package.

https://github.com/coreos/etcd/blob/e3da56a8dfb88c7ee8f6c282c986fb5154700910/embed/etcd.go#L518

Another way that *grpc.Server.Stop could have been missed is when sctx.registerGateway fails, but this is also hard to test (grpc.Dial takes context, but this server side context is not exported).

@codecov-io
Copy link

codecov-io commented Dec 8, 2017

Codecov Report

Merging #8992 into master will decrease coverage by 0.06%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8992      +/-   ##
==========================================
- Coverage   76.03%   75.96%   -0.07%     
==========================================
  Files         359      359              
  Lines       29819    29825       +6     
==========================================
- Hits        22672    22656      -16     
- Misses       5563     5585      +22     
  Partials     1584     1584
Impacted Files Coverage Δ
embed/serve.go 69.62% <86.66%> (+1.41%) ⬆️
clientv3/leasing/util.go 91.66% <0%> (-6.67%) ⬇️
clientv3/concurrency/session.go 88.63% <0%> (-4.55%) ⬇️
clientv3/leasing/cache.go 87.77% <0%> (-3.89%) ⬇️
pkg/testutil/recorder.go 77.77% <0%> (-3.71%) ⬇️
proxy/grpcproxy/watch.go 91.92% <0%> (-3.11%) ⬇️
etcdserver/api/v3rpc/lease.go 83.82% <0%> (-2.95%) ⬇️
pkg/netutil/netutil.go 63.21% <0%> (-2.3%) ⬇️
clientv3/leasing/kv.go 89.26% <0%> (-1.68%) ⬇️
lease/leasehttp/http.go 63.97% <0%> (-1.48%) ⬇️
... and 15 more

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 e3da56a...9744e1e. Read the comment docs.

embed/serve.go Outdated
@@ -101,6 +101,7 @@ func (sctx *serveCtx) serve(
opts := []grpc.DialOption{grpc.WithInsecure()}
gwmux, err := sctx.registerGateway(opts)
if err != nil {
gs.Stop()
Copy link
Contributor

Choose a reason for hiding this comment

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

probably make error as named return. and we always try to stop gs if it is not nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

If serve errors before *grpc.Server is sent to serversC,
it should be closed manually.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
@xiang90
Copy link
Contributor

xiang90 commented Dec 9, 2017

lgtm

@gyuho gyuho merged commit a7f1fbe into etcd-io:master Dec 9, 2017
@gyuho gyuho deleted the server-close branch December 9, 2017 03:54
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.

None yet

3 participants