Repository: zookeeper Updated Branches: refs/heads/branch-3.5 39fbc58f1 -> bdfa704a6
ZOOKEEPER-3162: Broken lock semantics in C client lock-recipe. 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. Author: Andrea Reale <[email protected]> Reviewers: [email protected] Closes #662 from andreareale/ZOOKEEPER-3162 and squashes the following commits: 09f7ff4ed [Andrea Reale] Fixes deadlock in zoo_lock_operation 670d25a8e [Andrea Reale] Fix return semantics of zkr_lock_lock 2a1d66c4b [Andrea Reale] Bugfix on zookeeper-recipes-lock C implementation a9b6a1a09 [Andrea Reale] Fix wrong include path for C recipes (cherry picked from commit 477fa0724fa66cc41d14e8a974ab4ac2a1b68433) Signed-off-by: Andor Molnar <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/bdfa704a Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/bdfa704a Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/bdfa704a Branch: refs/heads/branch-3.5 Commit: bdfa704a62d7da7cee6d369719918fb81db4ff03 Parents: 39fbc58 Author: Andrea Reale <[email protected]> Authored: Wed Nov 7 15:54:43 2018 -0800 Committer: Andor Molnar <[email protected]> Committed: Wed Nov 7 15:54:54 2018 -0800 ---------------------------------------------------------------------- .../zookeeper-recipes-lock/build.xml | 2 +- .../src/main/c/configure.ac | 4 +- .../src/main/c/src/zoo_lock.c | 60 +++++++++++++------- .../src/main/c/tests/TestClient.cc | 1 + .../zookeeper-recipes-queue/build.xml | 2 +- .../src/main/c/configure.ac | 4 +- 6 files changed, 47 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-lock/build.xml ---------------------------------------------------------------------- diff --git a/zookeeper-recipes/zookeeper-recipes-lock/build.xml b/zookeeper-recipes/zookeeper-recipes-lock/build.xml index 6d7d736..ea7b37f 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/build.xml +++ b/zookeeper-recipes/zookeeper-recipes-lock/build.xml @@ -52,7 +52,7 @@ <target name="compile-test" depends="compile"> <property name="target.jdk" value="${ant.java.version}" /> - <property name="src.test.local" location="${basedir}/test" /> + <property name="src.test.local" location="${basedir}/src/test/java" /> <mkdir dir="${build.test}"/> <javac srcdir="${src.test.local}" destdir="${build.test}" http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/configure.ac ---------------------------------------------------------------------- diff --git a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/configure.ac b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/configure.ac index 31c5406..53b2ea5 100644 --- a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/configure.ac +++ b/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/configure.ac @@ -48,8 +48,8 @@ DX_PS_FEATURE(OFF) DX_INIT_DOXYGEN([zookeeper-locks],[c-doc.Doxyfile],[docs]) -ZOOKEEPER_PATH=${BUILD_PATH}/../../../../zookeeper-client/zookeeper-client-c -ZOOKEEPER_LD=-L${BUILD_PATH}/../../../../zookeeper-client/zookeeper-client-c\ -lzookeeper_mt +ZOOKEEPER_PATH=${BUILD_PATH}/../../../../../zookeeper-client/zookeeper-client-c +ZOOKEEPER_LD=-L${BUILD_PATH}/../../../../../zookeeper-client/zookeeper-client-c\ -lzookeeper_mt AC_SUBST(ZOOKEEPER_PATH) AC_SUBST(ZOOKEEPER_LD) http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/src/zoo_lock.c ---------------------------------------------------------------------- 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 74a115f..5721d4e 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) { @@ -128,10 +132,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 +148,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 +287,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); @@ -300,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; } @@ -315,7 +335,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); @@ -364,7 +384,7 @@ ZOOAPI int zkr_lock_lock(zkr_lock_mutex_t *mutex) { } } pthread_mutex_unlock(&(mutex->pmutex)); - return zkr_lock_isowner(mutex); + return 0; } http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-lock/src/main/c/tests/TestClient.cc ---------------------------------------------------------------------- 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 2cc56cf..7a7675a 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 <cstring> #include <list> +#include <unistd.h> #include <zookeeper.h> #include <zoo_lock.h> http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-queue/build.xml ---------------------------------------------------------------------- diff --git a/zookeeper-recipes/zookeeper-recipes-queue/build.xml b/zookeeper-recipes/zookeeper-recipes-queue/build.xml index c7984ec..289c0f9 100644 --- a/zookeeper-recipes/zookeeper-recipes-queue/build.xml +++ b/zookeeper-recipes/zookeeper-recipes-queue/build.xml @@ -52,7 +52,7 @@ <target name="compile-test" depends="compile"> <property name="target.jdk" value="${ant.java.version}" /> - <property name="src.test.local" location="${basedir}/test" /> + <property name="src.test.local" location="${basedir}/src/test/java" /> <mkdir dir="${build.test}"/> <javac srcdir="${src.test.local}" destdir="${build.test}" http://git-wip-us.apache.org/repos/asf/zookeeper/blob/bdfa704a/zookeeper-recipes/zookeeper-recipes-queue/src/main/c/configure.ac ---------------------------------------------------------------------- diff --git a/zookeeper-recipes/zookeeper-recipes-queue/src/main/c/configure.ac b/zookeeper-recipes/zookeeper-recipes-queue/src/main/c/configure.ac index 23fa8c9..ede2480 100644 --- a/zookeeper-recipes/zookeeper-recipes-queue/src/main/c/configure.ac +++ b/zookeeper-recipes/zookeeper-recipes-queue/src/main/c/configure.ac @@ -48,8 +48,8 @@ DX_PS_FEATURE(OFF) DX_INIT_DOXYGEN([zookeeper-queues],[c-doc.Doxyfile],[docs]) -ZOOKEEPER_PATH=${BUILD_PATH}/../../../../zookeeper-client/zookeeper-client-c -ZOOKEEPER_LD=-L${BUILD_PATH}/../../../../zookeeper-client/zookeeper-client-c\ -lzookeeper_mt +ZOOKEEPER_PATH=${BUILD_PATH}/../../../../../zookeeper-client/zookeeper-client-c +ZOOKEEPER_LD=-L${BUILD_PATH}/../../../../../zookeeper-client/zookeeper-client-c\ -lzookeeper_mt AC_SUBST(ZOOKEEPER_PATH) AC_SUBST(ZOOKEEPER_LD)
