Merge "Handles spurious wake-ups in pthread_join()"
diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c
index e30fa9d..fb14097 100644
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -130,23 +130,13 @@
thread->tls = NULL;
}
- /* the join_count field is used to store the number of threads waiting for
- * the termination of this thread with pthread_join(),
- *
- * if it is positive we need to signal the waiters, and we do not touch
- * the count (it will be decremented by the waiters, the last one will
- * also remove/free the thread structure
- *
- * if it is zero, we set the count value to -1 to indicate that the
- * thread is in 'zombie' state: it has stopped executing, and its stack
- * is gone (as well as its TLS area). when another thread calls pthread_join()
- * on it, it will immediately free the thread and return.
- */
+ /* Indicate that the thread has exited for joining threads. */
+ thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
thread->return_value = retval;
- if (thread->join_count > 0) {
- pthread_cond_broadcast(&thread->join_cond);
- } else {
- thread->join_count = -1; /* zombie thread */
+
+ /* Signal the joining thread if present. */
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+ pthread_cond_signal(&thread->join_cond);
}
}
pthread_mutex_unlock(&gThreadListLock);
diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index 70a9bf5..19009e7 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -110,7 +110,6 @@
}
pthread_cond_init(&thread->join_cond, NULL);
- thread->join_count = 0;
thread->cleanup_stack = NULL;
if (add_to_thread_list) {
diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp
index 63f5809..95f11ac 100644
--- a/libc/bionic/pthread_detach.cpp
+++ b/libc/bionic/pthread_detach.cpp
@@ -40,7 +40,7 @@
return EINVAL; // Already detached.
}
- if (thread->join_count > 0) {
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
return 0; // Already being joined; silently do nothing, like glibc.
}
diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 316a14a..e34788b 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -42,7 +42,6 @@
pid_t tid;
bool allocated_on_heap;
pthread_cond_t join_cond;
- int join_count;
void* return_value;
int internal_flags;
__pthread_cleanup_t* cleanup_stack;
@@ -64,9 +63,18 @@
__LIBC_HIDDEN__ void pthread_key_clean_all(void);
__LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread);
+/* Has the thread been detached by a pthread_join or pthread_detach call? */
#define PTHREAD_ATTR_FLAG_DETACHED 0x00000001
+
+/* Was the thread's stack allocated by the user rather than by us? */
#define PTHREAD_ATTR_FLAG_USER_STACK 0x00000002
+/* Has the thread been joined by another thread? */
+#define PTHREAD_ATTR_FLAG_JOINED 0x00000004
+
+/* Has the thread already exited but not been joined? */
+#define PTHREAD_ATTR_FLAG_ZOMBIE 0x00000008
+
__LIBC_HIDDEN__ extern pthread_internal_t* gThreadList;
__LIBC_HIDDEN__ extern pthread_mutex_t gThreadListLock;
diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp
index e6acc34..7e022c2 100644
--- a/libc/bionic/pthread_join.cpp
+++ b/libc/bionic/pthread_join.cpp
@@ -30,7 +30,7 @@
#include "pthread_accessor.h"
-int pthread_join(pthread_t t, void ** ret_val) {
+int pthread_join(pthread_t t, void** ret_val) {
if (t == pthread_self()) {
return EDEADLK;
}
@@ -44,25 +44,19 @@
return EINVAL;
}
- // Wait for thread death when needed.
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+ return EINVAL;
+ }
- // If the 'join_count' is negative, this is a 'zombie' thread that
- // is already dead and without stack/TLS. Otherwise, we need to increment 'join-count'
- // and wait to be signaled
- int count = thread->join_count;
- if (count >= 0) {
- thread->join_count += 1;
+ // Signal our intention to join, and wait for the thread to exit.
+ thread->attr.flags |= PTHREAD_ATTR_FLAG_JOINED;
+ while ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) {
pthread_cond_wait(&thread->join_cond, &gThreadListLock);
- count = --thread->join_count;
}
if (ret_val) {
*ret_val = thread->return_value;
}
- // Remove thread from thread list when we're the last joiner or when the
- // thread was already a zombie.
- if (count <= 0) {
- _pthread_internal_remove_locked(thread.get());
- }
+ _pthread_internal_remove_locked(thread.get());
return 0;
}
diff --git a/libc/bionic/pthread_key.cpp b/libc/bionic/pthread_key.cpp
index c793fc6..2ae6519 100644
--- a/libc/bionic/pthread_key.cpp
+++ b/libc/bionic/pthread_key.cpp
@@ -212,16 +212,13 @@
// Clear value in all threads.
pthread_mutex_lock(&gThreadListLock);
for (pthread_internal_t* t = gThreadList; t != NULL; t = t->next) {
- // Avoid zombie threads with a negative 'join_count'. These are really
- // already dead and don't have a TLS area anymore.
-
+ // Skip zombie threads. They don't have a valid TLS area any more.
// Similarly, it is possible to have t->tls == NULL for threads that
// were just recently created through pthread_create() but whose
// startup trampoline (__thread_entry) hasn't been run yet by the
- // scheduler. t->tls will also be NULL after it's stack has been
+ // scheduler. t->tls will also be NULL after a thread's stack has been
// unmapped but before the ongoing pthread_join() is finished.
- // so check for this too.
- if (t->join_count < 0 || !t->tls) {
+ if ((t->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) || t->tls == NULL) {
continue;
}
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index a86cadc..d754312 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -317,3 +317,25 @@
ASSERT_EQ(ESRCH, pthread_kill(dead_thread, 0));
}
+
+TEST(pthread, pthread_join__multijoin) {
+ bool done = false;
+
+ pthread_t t1;
+ ASSERT_EQ(0, pthread_create(&t1, NULL, SpinFn, &done));
+
+ pthread_t t2;
+ ASSERT_EQ(0, pthread_create(&t2, NULL, JoinFn, reinterpret_cast<void*>(t1)));
+
+ sleep(1); // (Give t2 a chance to call pthread_join.)
+
+ // Multiple joins to the same thread should fail.
+ ASSERT_EQ(EINVAL, pthread_join(t1, NULL));
+
+ done = true;
+
+ // ...but t2's join on t1 still goes ahead (which we can tell because our join on t2 finishes).
+ void* join_result;
+ ASSERT_EQ(0, pthread_join(t2, &join_result));
+ ASSERT_EQ(0, reinterpret_cast<int>(join_result));
+}
