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

Dropped error in etcd/v2 lets multiple clients think they've acquired a lock. #50

Open
jlhawn opened this issue Dec 19, 2017 · 1 comment

Comments

@jlhawn
Copy link

jlhawn commented Dec 19, 2017

There's some bad error checking around here:

libkv/store/etcd/v2/etcd.go

Lines 528 to 534 in 5e4bb28

if err != nil {
if etcdError, ok := err.(etcd.Error); ok {
if etcdError.Code != etcd.ErrorCodeNodeExist {
return nil, err
}
setOpts.PrevIndex = ^uint64(0)
}

Where if the error is not of type etcd.Error then it is ignored completely. Then, another request to Set() the key is made with the PrevIndex value not set. This will just overwrite whatever value was at that key as long as it already exists. This results in two processes thinking they have acquired the lock at the same time. This only occurs under the condition that the first Set call fails due to some transient connection issue and the second call succeeds. We have seen this occur several times.

Why is there even a second call to Set() the key? Why not immediately go into a wait loop if the first Set() attempt results in etcd.ErrorCodeNodeExist?

@abronan
Copy link
Owner

abronan commented Dec 19, 2017

Thank you for the report @jlhawn ,

etcdv2 backend was not the core of my focus with this fork as this was deprecated with the support of etcdv3 client. I would advise switching because etcdv3 includes a proper locking interface while this one was just a hack to circumvent a missing lock implementation with etcdv2.

Returning to the core of the issue, I think we assumed that if the first call to Set returned an error, then it was unlikely that the second would succeed. But the scenario you describe is indeed possible so returning an error on the first call to Set if it fails and the error is different than etcd.Error makes sense. With a basic retry mechanism outside of the call to Lock, this would prevent the scenario you described.

This should be easy enough to fix, feel free to submit a patch or I'll do this if I have some free time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants