fix(bthread): Destroy pooled KeyTables outside cache locks

src/bthread/key.cpp

@@ -257,6 +257,24 @@ class BAIDU_CACHELINE_ALIGNMENT KeyTableList {
         _length++;
     }
 
+    // Transfer the entire chain without invoking KeyTable destructors. The
+    // caller uses this while holding the cache-management locks, then destroys
+    // the receiving list after releasing those locks.
+    void append_list(KeyTableList* other) {
+        if (other->_head == nullptr) {
+            return;
+        }
+        if (_head == nullptr) {
+            _head = other->_head;
+        } else {
+            _tail->next = other->_head;
+        }
+        _tail = other->_tail;
+        _length += other->_length;
+        other->_head = other->_tail = nullptr;
+        other->_length = 0;
+    }
+
     KeyTable* remove_front() {
         if (_head == nullptr) {
             return nullptr;
@@ -478,14 +496,32 @@ int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) {
         return EINVAL;
     }
     bthread::KeyTable* saved_free_keytables = nullptr;
-    pthread_rwlock_wrlock(&pool->rwlock);
-    pool->destroyed = 1;
-    pool->size = 0;
-    delete (butil::ThreadLocal<bthread::KeyTableList>*)pool->list;
-    saved_free_keytables = (bthread::KeyTable*)pool->free_keytables;
-    pool->list = nullptr;
-    pool->free_keytables = nullptr;
-    pthread_rwlock_unlock(&pool->rwlock);
+    butil::ThreadLocal<bthread::KeyTableList>* lists = nullptr;
+    {
+        bthread::KeyTableList detached;
+        pthread_rwlock_wrlock(&pool->rwlock);
+        pool->destroyed = 1;
+        pool->size = 0;
+        lists = (butil::ThreadLocal<bthread::KeyTableList>*)pool->list;
+        if (lists) {
+            // for_each() holds ThreadLocal's internal mutex. Only detach list
+            // nodes here: deleting a KeyTable can invoke a user TLS destructor.
+            lists->for_each([&detached](bthread::KeyTableList* list) {
+                detached.append_list(list);
+            });
+        }
+        saved_free_keytables = (bthread::KeyTable*)pool->free_keytables;
+        // After destroyed is set, borrowers and returners stop using these
+        // cache entries. Detaching their roots while holding the pool lock
+        // makes it safe to release both locks before running destructors.
+        pool->list = nullptr;
+        pool->free_keytables = nullptr;
+        pthread_rwlock_unlock(&pool->rwlock);
+        // Destroy the detached tables after releasing both cache locks. This
+        // scope ends before lists is deleted, so its destructor sees only
+        // empty KeyTableLists and cannot invoke user TLS destructors.
+    }
+    delete lists;
 
     // Cheat get/setspecific and destroy the keytables.
     bthread::TaskGroup* g =

test/bthread_key_unittest.cpp

@@ -33,6 +33,7 @@ namespace bthread {
 DECLARE_uint32(key_table_list_size);
 DECLARE_uint32(borrow_from_globle_size);
 class KeyTable;
+class KeyTableList;
 // defined in bthread/key.cpp
 extern void return_keytable(bthread_keytable_pool_t*, KeyTable*);
 extern KeyTable* borrow_keytable(bthread_keytable_pool_t*);
@@ -428,6 +429,56 @@ TEST(KeyTest, using_pool) {
     ASSERT_EQ(0, bthread_key_delete(key));
 }
 
+struct PoolDestructorLockCheck {
+    bthread_keytable_pool_t* pool;
+    butil::ThreadLocal<KeyTableList>* lists;
+    int pool_lock_result;
+    int list_lock_result;
+    int calls;
+};
+
+static void check_pool_destructor_locks(void* arg) {
+    PoolDestructorLockCheck* check = static_cast<PoolDestructorLockCheck*>(arg);
+    ++check->calls;
+    // A successful try-lock proves that the TLS destructor is not running
+    // under either cache-management lock.
+    check->pool_lock_result = pthread_rwlock_trywrlock(&check->pool->rwlock);
+    if (check->pool_lock_result == 0) {
+        pthread_rwlock_unlock(&check->pool->rwlock);
+    }
+    check->list_lock_result = pthread_mutex_trylock(&check->lists->_mutex);
+    if (check->list_lock_result == 0) {
+        pthread_mutex_unlock(&check->lists->_mutex);
+    }
+}
+
+static void* create_pool_destructor_lock_check(const void* arg) {
+    return const_cast<void*>(arg);
+}
+
+TEST(KeyTest, pooled_destructor_runs_outside_cache_locks) {
+    GFLAGS_NAMESPACE::FlagSaver flag_saver;
+    bthread::FLAGS_borrow_from_globle_size = 1;
+    bthread_keytable_pool_t pool;
+    ASSERT_EQ(0, bthread_keytable_pool_init(&pool));
+    PoolDestructorLockCheck check = {
+        &pool, static_cast<butil::ThreadLocal<KeyTableList>*>(pool.list), -1, -1, 0
+    };
+    bthread_key_t key;
+    ASSERT_EQ(0, bthread_key_create(&key, check_pool_destructor_locks));
+    bthread_keytable_pool_reserve(&pool, 1, key,
+                                  create_pool_destructor_lock_check, &check);
+    KeyTable* kt = borrow_keytable(&pool);
+    ASSERT_NE(nullptr, kt);
+    return_keytable(&pool, kt);
+    ASSERT_EQ(0, bthread_keytable_pool_destroy(&pool));
+    EXPECT_EQ(1, check.calls);
+    EXPECT_EQ(0, check.pool_lock_result);
+    EXPECT_EQ(0, check.list_lock_result);
+    EXPECT_EQ(0, bthread_key_delete(key));
+    EXPECT_EQ(0, pthread_rwlock_destroy(&pool.rwlock));
+}
+
 bthread_keytable_pool_t test_pool;
 struct PoolData2 {
     bthread_key_t key;