From a9b6a1a09f98a60a900fc98c99311604ea98952e 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 6d7d7360b20..ea7b37f08b2 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 | 34 ++++++++++++++----- .../src/main/c/tests/TestClient.cc | 1 + 2 files changed, 26 insertions(+), 9 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 74a115f79cf..da1d5f74262 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; } @@ -267,7 +283,7 @@ static int zkr_lock_operation(zkr_lock_mutex_t *mutex, struct timespec *ts) { // do not want to retry the create since // we would end up creating more than one child if (ret != ZOK) { - LOG_WARN(LOGCALLBACK(zh), ("could not create zoo node %s", buf)); + LOG_WARN(LOGCALLBACK(zh), "could not create zoo node %s", buf); return ret; } mutex->id = getName(retbuf); @@ -315,7 +331,7 @@ static int zkr_lock_operation(zkr_lock_mutex_t *mutex, struct timespec *ts) { // this is the case when we are the owner // of the lock if (strcmp(mutex->id, owner_id) == 0) { - LOG_DEBUG(LOGCALLBACK(zh), ("got the zoo lock owner - %s", mutex->id)); + LOG_DEBUG(LOGCALLBACK(zh), "got the zoo lock owner - %s", mutex->id); mutex->isOwner = 1; if (mutex->completion != NULL) { mutex->completion(0, mutex->cbdata); 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 670d25a8ef5b3a3a28a4f837a961819d6b52a5fd 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 da1d5f74262..5422c6a28a1 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 09f7ff4ed14053c50a05168d621b3c3a70a4ef43 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 5422c6a28a1..5721d4e8012 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(LOGCALLBACK(zh), ("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(LOGCALLBACK(zh), ("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; }