You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by andreareale <gi...@git.apache.org> on 2018/10/09 15:42:22 UTC
[GitHub] zookeeper pull request #662: ZOOKEEPER-3162
GitHub user andreareale opened a pull request:
https://github.com/apache/zookeeper/pull/662
ZOOKEEPER-3162
This PR fixes a few issues with the C client lock-recipe, as documented in more detailed in [ZOOKEEPER-3162](https://issues.apache.org/jira/browse/ZOOKEEPER-3162) on JIRA.
Details are also provided in the individual commits, but in short:
- Fix a bug in the choice of the predecessor node while trying to acquire the lock
- Fix a possible deadlock in zkr_lock_operation
- Fix the return value of zkr_lock_lock to abide to the prescribed semantics.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ibm-research-ireland/zookeeper ZOOKEEPER-3162
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/662.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #662
----
commit f5b0e09820fa0713d64119e4ba70fe0d07533a96
Author: Andrea Reale <re...@...>
Date: 2018-10-09T09:23:50Z
Fix wrong include path for C recipes
Signed-off-by: Andrea Reale <re...@ie.ibm.com>
commit d6e1953840f56c2c42948ff1958be779465e2bb9
Author: Andrea Reale <re...@...>
Date: 2018-08-03T16:35:56Z
Bugfix on zookeeper-recipes-lock C implementation
- Fixes ZOOKEEPER-2408,ZOOKEEPER-2038, and (partly) ZOOKEEPER-2878
- Fixes child_floor by using strcmp_suffix instead of strcmp. This will
do the correct lookups within the sorted waiters list. Without this
the lock semantics are broken: e.g., assuming that session A < session
B (alfanumerically), if a thread from session B holds the lock, and a
thread from session A tries to acquire it it will find no predecessor
in child_floor and get the lock as well.
- Uses binary search to find child_floor (optimization)
Signed-off-by: Andrea Reale <re...@ie.ibm.com>
commit e95da861ba0340949baa65b6f143d8a4e67aee6c
Author: Andrea Reale <re...@...>
Date: 2018-08-15T16:35:07Z
Fixes deadlock in zoo_lock_operation
zkr_lock_operation is always called by holding the mutex associated to
the client lock. In some cases, zkr_lock_operaton may decide to give-up
locking and call zkr_lock_unlock to release the lock. When this happens,
it will try to acquire again the same phtread mutex, which will lead to
a deadlock.
This commit fixes the issue by calling a non-protected version of
zkr_lock_unlock from within zkr_lock_operatino.
Signed-off-by: Andrea Reale <re...@ie.ibm.com>
commit 51ac01ecd06aca9d368519ff925a3539ddef35b4
Author: Andrea Reale <re...@...>
Date: 2018-10-09T14:52:11Z
Fix return semantics of zkr_lock_lock
Returns 0 (on no-errors) from zkr_lock_lock. This matches the
API documentation. Additionall, returning zkr_lock_isowner is also
semantically wrong: a caller might be enqueued for lock acquision and
still not the owner; in such a case the function should still return 0
because that is the expected behavior.
Signed-off-by: Andrea Reale <re...@ie.ibm.com>
----
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by andreareale <gi...@git.apache.org>.
Github user andreareale commented on the issue:
https://github.com/apache/zookeeper/pull/662
retest this please
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/662
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2397/
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/662
Looks good, thanks @andreareale
The C test fail looks like it's the flaky one:
*** Error in `./zktest-mt': free(): invalid pointer: 0x00002b913dcd5000 ***
I think there's already a fix underway.
This should also go into 3.5 as well (possibly to 3.4 especially the include path fix).
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by andreareale <gi...@git.apache.org>.
Github user andreareale commented on the issue:
https://github.com/apache/zookeeper/pull/662
Hi @nkalmar, @tamaashu and thanks for the reviews. Any chance to see this merged any time soon?
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/662
retest this please
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/662
Committed to 3.5 and master branches. Thanks @andreareale !
Please create separate pull request for branch-3.4
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by andreareale <gi...@git.apache.org>.
Github user andreareale commented on the issue:
https://github.com/apache/zookeeper/pull/662
Thanks @nkalmar. There were some additional path issues in the recipes ant test targets. Updated the PR to fix them as well. Please, let me know if there is anything I can do to help with merging.
---
[GitHub] zookeeper pull request #662: ZOOKEEPER-3162. Broken lock semantics in C clie...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/662
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/662
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2394/
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/662
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2417/
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by tamaashu <gi...@git.apache.org>.
Github user tamaashu commented on the issue:
https://github.com/apache/zookeeper/pull/662
Since I'm not a committer I cannot do so. @anmolnar @hanm could you please review this PR? Thanks.
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by tamaashu <gi...@git.apache.org>.
Github user tamaashu commented on the issue:
https://github.com/apache/zookeeper/pull/662
looks okay, thanks for fixing build issues too
---
[GitHub] zookeeper issue #662: ZOOKEEPER-3162. Broken lock semantics in C client lock...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/662
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2437/
---