Clean up the area of pthread key destruction.
The previous commentary (based on code inspection) was wrong. We do correctly implement the POSIX semantics of only calling pthread key destructor functions for non-null values. This change updates the documentation to be correct, adds the missing unit tests that prove that the implementation is correct, and changes the implementation to make it more obviously correct (and so we can add an explicit code comment pointing out the relevant null check).
Change-Id: I04511021233da8aac80c010eb111ffe615171611
diff --git a/libc/bionic/pthread_key.cpp b/libc/bionic/pthread_key.cpp
index 53f0f11..f8c765d 100644
--- a/libc/bionic/pthread_key.cpp
+++ b/libc/bionic/pthread_key.cpp
@@ -83,11 +83,18 @@
size_t called_destructor_count = 0;
for (size_t i = 0; i < BIONIC_PTHREAD_KEY_COUNT; ++i) {
uintptr_t seq = atomic_load_explicit(&key_map[i].seq, memory_order_relaxed);
- if (SeqOfKeyInUse(seq) && seq == key_data[i].seq && key_data[i].data != nullptr) {
- // Other threads may be calling pthread_key_delete/pthread_key_create while current thread
- // is exiting. So we need to ensure we read the right key_destructor.
+ if (SeqOfKeyInUse(seq) && seq == key_data[i].seq) {
+ // POSIX explicitly says that the destructor is only called if the
+ // thread has a non-null value for the key.
+ if (key_data[i].data == nullptr) {
+ continue;
+ }
+
+ // Other threads can call pthread_key_delete()/pthread_key_create()
+ // while this thread is exiting, so we need to ensure we read the right
+ // key_destructor.
// We can rely on a user-established happens-before relationship between the creation and
- // use of pthread key to ensure that we're not getting an earlier key_destructor.
+ // use of a pthread key to ensure that we're not getting an earlier key_destructor.
// To avoid using the key_destructor of the newly created key in the same slot, we need to
// recheck the sequence number after reading key_destructor. As a result, we either see the
// right key_destructor, or the sequence number must have changed when we reread it below.
@@ -107,7 +114,6 @@
// function is responsible for manually releasing the corresponding data.
void* data = key_data[i].data;
key_data[i].data = nullptr;
-
(*key_destructor)(data);
++called_destructor_count;
}
@@ -163,13 +169,13 @@
key &= ~KEY_VALID_FLAG;
uintptr_t seq = atomic_load_explicit(&key_map[key].seq, memory_order_relaxed);
pthread_key_data_t* data = &get_thread_key_data()[key];
- // It is user's responsibility to synchornize between the creation and use of pthread keys,
+ // It is the user's responsibility to synchronize between the creation and use of pthread keys,
// so we use memory_order_relaxed when checking the sequence number.
if (__predict_true(SeqOfKeyInUse(seq) && data->seq == seq)) {
return data->data;
}
- // We arrive here when current thread holds the seq of an deleted pthread key. So the
- // data is for the deleted pthread key, and should be cleared.
+ // We arrive here when the current thread holds the seq of a deleted pthread key.
+ // The data is for the deleted pthread key, and should be cleared.
data->data = nullptr;
return nullptr;
}
diff --git a/libc/include/pthread.h b/libc/include/pthread.h
index f3b7068..5a3376a 100644
--- a/libc/include/pthread.h
+++ b/libc/include/pthread.h
@@ -187,8 +187,7 @@
* different language, you should consider similar implementation choices and
* avoid a direct one-to-one mapping from thread locals to pthread keys.
*
- * On Android, the destructor function is called even for null values,
- * even though POSIX specifies that it is only called for non-null values.
+ * The destructor function is only called for non-null values.
*
* Returns 0 on success and returns an error number on failure.
*/
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index 5ce7d4d..680ef6e 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -59,6 +59,39 @@
ASSERT_EQ(EINVAL, pthread_key_delete(key));
}
+static std::vector<void*> example_key_destructor_data;
+static pthread_key_t example_key;
+static void example_key_destructor(void *data) {
+ // By the time the destructor function is running,
+ // this thread's value for the key should have been zeroed.
+ ASSERT_EQ(NULL, pthread_getspecific(example_key));
+
+ // Store the value so we can check we got the expected result.
+ example_key_destructor_data.push_back(data);
+}
+
+TEST(pthread, pthread_key_destructors) {
+ ASSERT_EQ(0, pthread_key_create(&example_key, example_key_destructor));
+
+ // Check that the destructor isn't called for a default null value.
+ std::thread([]() {}).join();
+ ASSERT_TRUE(example_key_destructor_data.empty());
+
+ // Check that the destructor isn't called for an explicit null value.
+ std::thread([]() {
+ ASSERT_EQ(0, pthread_setspecific(example_key, (void*) 1234));
+ ASSERT_EQ(0, pthread_setspecific(example_key, nullptr));
+ }).join();
+ ASSERT_TRUE(example_key_destructor_data.empty());
+
+ // Check that the destructor is called for a non-null value.
+ std::thread([]() { ASSERT_EQ(0, pthread_setspecific(example_key, (void*) 1234)); }).join();
+ ASSERT_EQ(1u, example_key_destructor_data.size());
+ ASSERT_EQ((void*) 1234, example_key_destructor_data[0]);
+
+ ASSERT_EQ(0, pthread_key_delete(example_key));
+}
+
TEST(pthread, pthread_keys_max) {
// POSIX says PTHREAD_KEYS_MAX should be at least _POSIX_THREAD_KEYS_MAX.
ASSERT_GE(PTHREAD_KEYS_MAX, _POSIX_THREAD_KEYS_MAX);