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

concurrency: make lock more reliable #11408

Merged
merged 1 commit into from Dec 4, 2019
Merged

concurrency: make lock more reliable #11408

merged 1 commit into from Dec 4, 2019

Conversation

@xiang90
Copy link
Contributor

xiang90 commented Dec 1, 2019

check the existence of the lock key before returning the lock api call.

/cc @mitake @aphyr @gyuho

@xiang90 xiang90 force-pushed the xiang90:lock branch 3 times, most recently from 2ff03bc to 7ab8b6b Dec 1, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #11408 into master will decrease coverage by <.01%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11408      +/-   ##
==========================================
- Coverage   64.58%   64.57%   -0.01%     
==========================================
  Files         403      403              
  Lines       37969    37976       +7     
==========================================
+ Hits        24522    24523       +1     
- Misses      11814    11815       +1     
- Partials     1633     1638       +5
Impacted Files Coverage Δ
clientv3/concurrency/mutex.go 62.16% <62.5%> (-0.53%) ⬇️
client/keys.go 76.88% <0%> (-14.58%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
clientv3/balancer/balancer.go 86.36% <0%> (-2.28%) ⬇️
clientv3/maintenance.go 40.81% <0%> (-2.05%) ⬇️
clientv3/balancer/resolver/endpoint/endpoint.go 82.6% <0%> (-1.74%) ⬇️
lease/leasehttp/http.go 64.23% <0%> (-1.46%) ⬇️
pkg/schedule/schedule.go 81.69% <0%> (-1.41%) ⬇️
... and 18 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 d1139d3...b14c7cd. Read the comment docs.

go func() {
defer close(m2Locked)
// wait until s1 is locks /my-lock/
if err2 = m2.Lock(context.TODO()); err == nil {

This comment has been minimized.

Copy link
@gyuho

gyuho Dec 2, 2019

Member

s/err == nil/err2 == nil/?

var err2 error
go func() {
defer close(m2Locked)
// wait until s1 is locks /my-lock/

This comment has been minimized.

Copy link
@gyuho

gyuho Dec 2, 2019

Member

m2 blocks since m1 already acquired lock /my-lock/?

@gyuho

This comment has been minimized.

Copy link
Member

gyuho commented Dec 2, 2019

Minor nits.

LGTM.

Thanks @xiang90 @aphyr!

return werr
}

m.hdr = gresp.Header

This comment has been minimized.

Copy link
@mitake

mitake Dec 2, 2019

Contributor

Swapping the assignment of hdr and checking the number of keys would be safer? In other places users won't see initialized hdr when locking is failed.

@xiang90 xiang90 force-pushed the xiang90:lock branch from 7ab8b6b to 36594d4 Dec 2, 2019
@xiang90 xiang90 force-pushed the xiang90:lock branch from 36594d4 to b14c7cd Dec 2, 2019
@xiang90

This comment has been minimized.

Copy link
Contributor Author

xiang90 commented Dec 2, 2019

@gyuho @mitake all fixed. tests passed.

@gyuho
gyuho approved these changes Dec 4, 2019
Copy link
Member

gyuho left a comment

lgtm, thx!

@gyuho gyuho merged commit e5356cc into etcd-io:master Dec 4, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
semaphoreci The build passed on Semaphore.
Details
@xiang90 xiang90 deleted the xiang90:lock branch Dec 4, 2019
@dubstack

This comment has been minimized.

Copy link

dubstack commented Dec 10, 2019

https://github.com/etcd-io/etcd/blob/master/clientv3/concurrency/mutex.go#L87
@gyuho @xiang90 The werr referenced above is now being swallowed which is not what we want.

@xiang90

This comment has been minimized.

Copy link
Contributor Author

xiang90 commented Dec 17, 2019

@dubstack good catch. fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.