Copilot commented on code in PR #3638:
URL: https://github.com/apache/brpc/pull/3638#discussion_r4344172640
##########
test/bthread_key_unittest.cpp:
##########
@@ -566,6 +567,78 @@ 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, but the test logic assumes
they block until `signal()` is called (e.g., workers call `start.wait()` /
`destroyed.wait()`). If `CountdownEvent` defaults to an initial count of 0, the
waits will not block and the test won’t actually enforce the intended ordering
(making it much less effective and potentially flaky). Initialize these
explicitly (e.g., `start(1)` and `destroyed(1)`) in the constructor initializer
list.
##########
test/bthread_key_unittest.cpp:
##########
@@ -566,6 +567,78 @@ 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);
Review Comment:
Calling GoogleTest `EXPECT_*` macros from worker threads is not reliably
thread-safe and can lead to flaky failures or corrupted test output. Prefer
recording failures into an `std::atomic<bool>` / shared error state (or return
an error code via `pthread_exit`) and then asserting in the main test thread
after `pthread_join()`.
##########
test/bthread_key_unittest.cpp:
##########
@@ -566,6 +567,78 @@ 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) {
Review Comment:
This test’s correctness depends on the runtime value of
`FLAGS_borrow_from_globle_size` (which can be overridden by test runners/CI
flags). If it’s set to 0 or an unexpectedly small value, the test may stop
exercising the intended “held across destruction + racing borrows” scenario (or
start failing spuriously). To make the test deterministic, set the flag to a
known value within the test (and restore it afterward), or avoid using the flag
to size `NTABLES`.
--
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]
