wrowe 2003/08/07 15:16:24
Modified: include/arch/unix apr_arch_thread_mutex.h
locks/unix thread_mutex.c
Log:
Revamp apr_thread_mutex to be as thread safe, and occasionally
as reentrant as possible. Switched to atomics to preserve the
incr/decr integrity.
Although we previously reset the thread_id to zero, it's been
pointed out on list that this is less than desireable. However,
negative one isn't necessarily a good choice because several
platforms have unsigned thread_t's.
Revision Changes Path
1.2 +3 -2 apr/include/arch/unix/apr_arch_thread_mutex.h
Index: apr_arch_thread_mutex.h
===================================================================
RCS file: /home/cvs/apr/include/arch/unix/apr_arch_thread_mutex.h,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- apr_arch_thread_mutex.h 6 Jan 2003 23:44:26 -0000 1.1
+++ apr_arch_thread_mutex.h 7 Aug 2003 22:16:24 -0000 1.2
@@ -60,6 +60,7 @@
#include "apr_general.h"
#include "apr_thread_mutex.h"
#include "apr_portable.h"
+#include "apr_atomic.h"
#if APR_HAVE_PTHREAD_H
#include <pthread.h>
@@ -69,8 +70,8 @@
struct apr_thread_mutex_t {
apr_pool_t *pool;
pthread_mutex_t mutex;
- apr_os_thread_t owner;
- int owner_ref;
+ volatile apr_os_thread_t owner;
+ volatile apr_atomic_t owner_ref;
char nested; /* a boolean */
};
#endif
1.19 +57 -23 apr/locks/unix/thread_mutex.c
Index: thread_mutex.c
===================================================================
RCS file: /home/cvs/apr/locks/unix/thread_mutex.c,v
retrieving revision 1.18
retrieving revision 1.19
diff -u -r1.18 -r1.19
--- thread_mutex.c 8 Jun 2003 13:42:20 -0000 1.18
+++ thread_mutex.c 7 Aug 2003 22:16:24 -0000 1.19
@@ -108,8 +108,13 @@
apr_status_t rv;
if (mutex->nested) {
+ /*
+ * Although threadsafe, this test is NOT reentrant.
+ * The thread's main and reentrant attempts will both mismatch
+ * testing the mutex is owned by this thread, so a deadlock is
expected.
+ */
if (apr_os_thread_equal(mutex->owner, apr_os_thread_current())) {
- mutex->owner_ref++;
+ apr_atomic_inc(mutex->owner_ref);
return APR_SUCCESS;
}
@@ -121,8 +126,17 @@
return rv;
}
+ if (apr_atomic_cas(&mutex->owner_ref, 1, 0) != 0) {
+ /* The owner_ref should be zero when the lock is not held,
+ * if owner_ref was non-zero we have a mutex reference bug.
+ * XXX: so now what?
+ */
+ mutex->owner_ref = 1;
+ }
+ /* Note; do not claim ownership until the owner_ref has been
+ * incremented; limits a subtle race in reentrant code.
+ */
mutex->owner = apr_os_thread_current();
- mutex->owner_ref = 1;
return rv;
}
else {
@@ -141,8 +155,14 @@
apr_status_t rv;
if (mutex->nested) {
+ /*
+ * Although threadsafe, this test is NOT reentrant.
+ * The thread's main and reentrant attempts will both mismatch
+ * testing the mutex is owned by this thread, so one will fail
+ * the trylock.
+ */
if (apr_os_thread_equal(mutex->owner, apr_os_thread_current())) {
- mutex->owner_ref++;
+ apr_atomic_inc(mutex->owner_ref);
return APR_SUCCESS;
}
@@ -154,8 +174,17 @@
return (rv == EBUSY) ? APR_EBUSY : rv;
}
+ if (apr_atomic_cas(&mutex->owner_ref, 1, 0) != 0) {
+ /* The owner_ref should be zero when the lock is not held,
+ * if owner_ref was non-zero we have a mutex reference bug.
+ * XXX: so now what?
+ */
+ mutex->owner_ref = 1;
+ }
+ /* Note; do not claim ownership until the owner_ref has been
+ * incremented; limits a subtle race in reentrant code.
+ */
mutex->owner = apr_os_thread_current();
- mutex->owner_ref = 1;
}
else {
rv = pthread_mutex_trylock(&mutex->mutex);
@@ -175,32 +204,37 @@
apr_status_t status;
if (mutex->nested) {
+ /*
+ * The code below is threadsafe and reentrant.
+ */
if (apr_os_thread_equal(mutex->owner, apr_os_thread_current())) {
- mutex->owner_ref--;
- if (mutex->owner_ref > 0)
+ /*
+ * This should never occur, and indicates an application error
+ */
+ if (mutex->owner_ref == 0) {
+ return APR_EINVAL;
+ }
+
+ if (apr_atomic_dec(mutex->owner_ref) != 0)
return APR_SUCCESS;
+ mutex->owner = 0;
}
- status = pthread_mutex_unlock(&mutex->mutex);
- if (status) {
-#ifdef PTHREAD_SETS_ERRNO
- status = errno;
-#endif
- return status;
+ /*
+ * This should never occur, and indicates an application error
+ */
+ else {
+ return APR_EINVAL;
}
-
- memset(&mutex->owner, 0, sizeof mutex->owner);
- mutex->owner_ref = 0;
- return status;
}
- else {
- status = pthread_mutex_unlock(&mutex->mutex);
+
+ status = pthread_mutex_unlock(&mutex->mutex);
#ifdef PTHREAD_SETS_ERRNO
- if (status) {
- status = errno;
- }
-#endif
- return status;
+ if (status) {
+ status = errno;
}
+#endif
+
+ return status;
}
APR_DECLARE(apr_status_t) apr_thread_mutex_destroy(apr_thread_mutex_t *mutex)
