Copilot commented on code in PR #3638:
URL: https://github.com/apache/brpc/pull/3638#discussion_r4344887558
##########
test/bthread_key_unittest.cpp:
##########
@@ -566,6 +567,80 @@ TEST(KeyTest, borrow_and_return_keytable_when_using_pool) {
ASSERT_EQ(0, bthread_keytable_pool_destroy(&test_pool));
}
+struct ConcurrentPoolData {
+ bthread_keytable_pool_t pool;
+ CountdownEvent ready;
+ CountdownEvent start;
+ CountdownEvent destroyed;
+ Counters counters;
+
+ explicit ConcurrentPoolData(int nworkers) : ready(nworkers) {}
Review Comment:
`start` and `destroyed` are default-constructed, and the test’s correctness
depends on their initial count (e.g., 1). If `CountdownEvent`’s default count
is 0 (or otherwise not 1), `start.wait()` / `destroyed.wait()` may not
synchronize as intended (or could deadlock). Make the initial counts explicit
(e.g., initialize `start(1)` and `destroyed(1)` in the constructor initializer
list).
##########
test/bthread_key_unittest.cpp:
##########
@@ -566,6 +567,80 @@ TEST(KeyTest, borrow_and_return_keytable_when_using_pool) {
ASSERT_EQ(0, bthread_keytable_pool_destroy(&test_pool));
}
+struct ConcurrentPoolData {
+ bthread_keytable_pool_t pool;
+ CountdownEvent ready;
+ CountdownEvent start;
+ CountdownEvent destroyed;
+ Counters counters;
+
+ explicit ConcurrentPoolData(int nworkers) : ready(nworkers) {}
+};
+
+static void* create_concurrent_pool_data(const void* arg) {
+ Counters* counters = const_cast<Counters*>(static_cast<const
Counters*>(arg));
+ counters->ncreate.fetch_add(1, butil::memory_order_relaxed);
+ return counters;
+}
+
+static void destroy_concurrent_pool_data(void* arg) {
+ static_cast<Counters*>(arg)->ndestroy.fetch_add(1,
butil::memory_order_relaxed);
+}
+
+static void* borrow_and_return_during_pool_destruction(void* arg) {
+ ConcurrentPoolData* data = static_cast<ConcurrentPoolData*>(arg);
+ // Keep a borrowed table across destruction as well as racing new borrows
+ // with destruction. The pool and key remain valid until all workers exit.
+ KeyTable* held = borrow_keytable(&data->pool);
+ EXPECT_NE(nullptr, held);
+ data->ready.signal();
+ data->start.wait();
+ for (int i = 0; i < 4096; ++i) {
+ KeyTable* kt = borrow_keytable(&data->pool);
+ return_keytable(&data->pool, kt);
+ }
+ data->destroyed.wait();
+ return_keytable(&data->pool, held);
+ return nullptr;
+}
+
+TEST(KeyTest, borrow_and_return_keytable_during_pool_destruction) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ bthread::FLAGS_borrow_from_globle_size = 20;
+ const size_t NWORKERS = 8;
+ const size_t NTABLES = NWORKERS * bthread::FLAGS_borrow_from_globle_size;
+ ConcurrentPoolData data(NWORKERS);
+ ASSERT_EQ(0, bthread_keytable_pool_init(&data.pool));
+ bthread_key_t key;
+ ASSERT_EQ(0, bthread_key_create(&key, destroy_concurrent_pool_data));
+ bthread_keytable_pool_reserve(&data.pool, NTABLES, key,
+ create_concurrent_pool_data, &data.counters);
+ ASSERT_EQ(NTABLES,
data.counters.ncreate.load(butil::memory_order_relaxed));
+
+ pthread_t workers[NWORKERS];
+ size_t nstarted = 0;
+ for (; nstarted < NWORKERS; ++nstarted) {
+ int rc = pthread_create(&workers[nstarted], nullptr,
+ borrow_and_return_during_pool_destruction,
&data);
+ EXPECT_EQ(0, rc);
+ if (rc != 0) {
+ data.ready.signal(NWORKERS - nstarted);
+ break;
Review Comment:
Using `EXPECT_EQ(0, rc)` and then continuing into `data.ready.wait()` can
still hang the test if thread creation fails in a way that prevents
already-started workers from reaching `ready.signal()` (or if fewer workers run
than expected). Prefer making thread creation a hard precondition (e.g.,
`ASSERT_EQ(0, rc)`), or if you want to tolerate failure, gate the rest of the
test on success and skip/return early to avoid potential hangs.
##########
src/bthread/key.cpp:
##########
@@ -334,9 +339,10 @@ KeyTable* borrow_keytable(bthread_keytable_pool_t* pool) {
return p;
}
}
+ has_free_keytables = pool->free_keytables != nullptr;
pthread_rwlock_unlock(&pool->rwlock);
}
- if (pool->free_keytables) {
+ if (has_free_keytables) {
Review Comment:
`has_free_keytables` is sampled under the read lock and then used as a gate
to decide whether to take the write lock. If it’s `false` here but another
thread returns/moves keytables to `pool->free_keytables` immediately after,
this call may skip the reuse path and fall through to slower/allocating
behavior (depending on the remainder of the function). Consider rechecking
`pool->free_keytables` under the write lock when the per-thread list is empty
(at least in the common “list empty” case), or using a lightweight retry
strategy, to reduce avoidable allocations under contention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
