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

fix(recipe): fix deadlock in r/w lock recipe #650

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

westphahl
Copy link
Contributor

@westphahl westphahl commented Jul 31, 2021

The lock must only consider contenders with a sequence number lower than
it's own sequence number as also stated in the Zookeeper recipe
description for shared locks
.

This wasn't working correctly as the ReadLock also considered WriteLocks
with a higher sequence number as contenders. This can lead to a deadlock
as described in #649.

Closes #649

jeblair
jeblair previously approved these changes Jul 31, 2021
Copy link
Contributor

@jeblair jeblair left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test case for #649?

@westphahl westphahl changed the title Fix possible deadlock in r/w lock recipe fix(recipe): fix deadlock in r/w lock recipe Aug 2, 2021
@westphahl
Copy link
Contributor Author

Is it possible to add a test case for #649?

Done.

@StephenSorriaux
Copy link
Member

Thank you, I will first fix the CI since travis-ci.org has ceased and then take a look at your PR.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

This looks fine to me from a code perspective. I haven't used this lock recipe at all, so afraid I can't comment on the design change. But your explanation seems lucid/makes sense.

@westphahl
Copy link
Contributor Author

@StephenSorriaux I rebased the fix in order to have the CI jobs running.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #650 (884cac3) into master (4042a85) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   94.44%   94.49%   +0.04%     
==========================================
  Files          57       57              
  Lines        8342     8375      +33     
==========================================
+ Hits         7879     7914      +35     
+ Misses        463      461       -2     
Impacted Files Coverage Δ
kazoo/recipe/lock.py 97.96% <100.00%> (+<0.01%) ⬆️
kazoo/tests/test_lock.py 99.30% <100.00%> (-0.15%) ⬇️
kazoo/protocol/connection.py 95.25% <0.00%> (+0.20%) ⬆️
kazoo/client.py 98.56% <0.00%> (+0.31%) ⬆️

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 4042a85...884cac3. Read the comment docs.

The lock must only consider contenders with a sequence number lower than
it's own sequence number as also stated in the Zookeeper recipe
description for shared locks[0].

This wasn't working correctly as the ReadLock also considered WriteLocks
with a higher sequence number as contenders. This can lead to a deadlock
as described in python-zk#649.

[0]: https://zookeeper.apache.org/doc/r3.7.0/recipes.html#Shared+Locks

Closes python-zk#649
@StephenSorriaux
Copy link
Member

Mh seems like Codecov has some trouble... Will retry the jobs tomorow

Copy link
Member

@StephenSorriaux StephenSorriaux left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this.

@StephenSorriaux StephenSorriaux merged commit 9bb8499 into python-zk:master Feb 7, 2022
@westphahl westphahl deleted the rw-deadlock branch February 7, 2022 14:05
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

Successfully merging this pull request may close these issues.

Deadlock with read/write lock recipe
6 participants