From 07d5b8e3fb2b3ea5df9df9ea22fd9706da15e1d9 Mon Sep 17 00:00:00 2001 From: Andrea Reale Date: Tue, 9 Oct 2018 10:23:50 +0100 Subject: [PATCH 1/4] Fix wrong include path for C recipes Signed-off-by: Andrea Reale --- zookeeper-recipes/zookeeper-recipes-lock/build.xml | 2 +- .../zookeeper-recipes-lock/src/main/c/configure.ac | 4 ++-- zookeeper-recipes/zookeeper-recipes-queue/build.xml | 2 +- .../zookeeper-recipes-queue/src/main/c/configure.ac | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/zookeeper-recipes/zookeeper-recipes-lock/build.xml b/zookeeper-recipes/zookeeper-recipes-lock/build.xml index ac8668b002c..840a5bbf593 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/build.xml +++ b/zookeeper-recipes/zookeeper-recipes-lock/build.xml @@ -52,7 +52,7 @@ - + - + Date: Fri, 3 Aug 2018 17:35:56 +0100 Subject: [PATCH 2/4] 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 --- .../src/main/c/src/zoo_lock.c | 30 ++++++++++++++----- .../src/main/c/tests/TestClient.cc | 1 + 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c index 8a6d81763f4..a0ac95ec19a 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c +++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c @@ -128,10 +128,14 @@ static void free_String_vector(struct String_vector *v) { } } +static int strcmp_suffix(const char *str1, const char *str2) { + return strcmp(strrchr(str1, '-')+1, strrchr(str2, '-')+1); +} + static int vstrcmp(const void* str1, const void* str2) { const char **a = (const char**)str1; const char **b = (const char**) str2; - return strcmp(strrchr(*a, '-')+1, strrchr(*b, '-')+1); + return strcmp_suffix(*a, *b); } static void sort_children(struct String_vector *vector) { @@ -140,12 +144,24 @@ static void sort_children(struct String_vector *vector) { static char* child_floor(char **sorted_data, int len, char *element) { char* ret = NULL; - int i =0; - for (i=0; i < len; i++) { - if (strcmp(sorted_data[i], element) < 0) { - ret = sorted_data[i]; - } - } + int targetpos = -1, s = 0, e = len -1; + + while ( targetpos < 0 && s <= e ) { + int const i = s + (e - s) / 2; + int const cmp = strcmp_suffix(sorted_data[i], element); + if (cmp < 0) { + s = i + 1; + } else if (cmp == 0) { + targetpos = i; + } else { + e = i - 1; + } + } + + if (targetpos > 0) { + ret = sorted_data[targetpos - 1]; + } + return ret; } diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/tests/TestClient.cc b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/tests/TestClient.cc index 2cc56cf344a..7a7675a98ed 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/tests/TestClient.cc +++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/tests/TestClient.cc @@ -28,6 +28,7 @@ using namespace std; #include #include +#include #include #include From 99f5959efeb8049c5318515b5d87b6e80add35c0 Mon Sep 17 00:00:00 2001 From: Andrea Reale Date: Tue, 9 Oct 2018 15:52:11 +0100 Subject: [PATCH 3/4] 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 --- .../zookeeper-recipes-lock/src/main/c/src/zoo_lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c index a0ac95ec19a..74e1beb144c 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c +++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c @@ -380,7 +380,7 @@ ZOOAPI int zkr_lock_lock(zkr_lock_mutex_t *mutex) { } } pthread_mutex_unlock(&(mutex->pmutex)); - return zkr_lock_isowner(mutex); + return 0; } From b33a0bf8545f089a9f29c43414125032a2b5c1d3 Mon Sep 17 00:00:00 2001 From: Andrea Reale Date: Wed, 15 Aug 2018 17:35:07 +0100 Subject: [PATCH 4/4] 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 --- .../src/main/c/src/zoo_lock.c | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c index 74e1beb144c..7b7ef226e59 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c +++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c @@ -74,11 +74,7 @@ ZOOAPI int zkr_lock_init_cb(zkr_lock_mutex_t *mutex, zhandle_t* zh, return 0; } -/** - * unlock the mutex - */ -ZOOAPI int zkr_lock_unlock(zkr_lock_mutex_t *mutex) { - pthread_mutex_lock(&(mutex->pmutex)); +static int _zkr_lock_unlock_nolock(zkr_lock_mutex_t *mutex) { zhandle_t *zh = mutex->zh; if (mutex->id != NULL) { int len = strlen(mutex->path) + strlen(mutex->id) + 2; @@ -106,15 +102,23 @@ ZOOAPI int zkr_lock_unlock(zkr_lock_mutex_t *mutex) { free(mutex->id); mutex->id = NULL; - pthread_mutex_unlock(&(mutex->pmutex)); return 0; } LOG_WARN(("not able to connect to server - giving up")); - pthread_mutex_unlock(&(mutex->pmutex)); return ZCONNECTIONLOSS; } + + return ZSYSTEMERROR; +} +/** + * unlock the mutex + */ +ZOOAPI int zkr_lock_unlock(zkr_lock_mutex_t *mutex) { + int ret = 0; + pthread_mutex_lock(&(mutex->pmutex)); + ret = _zkr_lock_unlock_nolock(mutex); pthread_mutex_unlock(&(mutex->pmutex)); - return ZSYSTEMERROR; + return ret; } static void free_String_vector(struct String_vector *v) { @@ -316,11 +320,11 @@ static int zkr_lock_operation(zkr_lock_mutex_t *mutex, struct timespec *ts) { if (ret != ZOK) { free_String_vector(vector); LOG_WARN(("unable to watch my predecessor")); - ret = zkr_lock_unlock(mutex); + ret = _zkr_lock_unlock_nolock(mutex); while (ret == 0) { //we have to give up our leadership // since we cannot watch out predecessor - ret = zkr_lock_unlock(mutex); + ret = _zkr_lock_unlock_nolock(mutex); } return ret; }