Copilot commented on code in PR #3673:
URL: https://github.com/apache/brpc/pull/3673#discussion_r4384543102
##########
test/bthread_semaphore_unittest.cpp:
##########
@@ -129,6 +134,91 @@ TEST(SemaphoreTest, trywait) {
ASSERT_EQ(0, bthread_sem_destroy(&sem));
}
+struct SemButexWaitArgs {
+ bthread_sem_t* sem;
+ int expected_value;
+ bthread::CountdownEvent* ready;
+ butil::atomic<int> nwoken{0};
+};
+
+void* sem_butex_waiter(void* arg) {
+ auto args = static_cast<SemButexWaitArgs*>(arg);
+ args->ready->signal();
+ const timespec deadline = butil::seconds_from_now(10);
+ // Wait only once so that excess wakeups cannot be hidden by sem_wait's
+ // retry loop. Do not consume the posted permits.
+ const int rc = bthread::butex_wait(args->sem->butex, args->expected_value,
+ &deadline);
+ if (rc == 0) {
+ args->nwoken.fetch_add(1, butil::memory_order_relaxed);
+ } else {
+ // The post may race with a waiter that has not yet been queued.
+ EXPECT_EQ(EWOULDBLOCK, errno);
+ }
+ return nullptr;
+}
+
+void check_post_wakeup_count(unsigned initial_value, size_t num, bool batch) {
+ SCOPED_TRACE(initial_value);
+ const int N = 8;
+ // Retry if too few waiters were queued before posting. This avoids relying
+ // on a fixed sleep to establish that the waiters have entered butex_wait.
+ for (int attempt = 0; attempt < 10; ++attempt) {
+ bthread_sem_t sem;
+ ASSERT_EQ(0, bthread_sem_init(&sem, initial_value));
+ bthread::CountdownEvent ready(2 * N);
+ SemButexWaitArgs args{&sem, static_cast<int>(initial_value), &ready};
+ bthread_t bthreads[N];
+ pthread_t pthreads[N];
+ for (int i = 0; i < N; ++i) {
+ ASSERT_EQ(0, bthread_start_background(
+ &bthreads[i], nullptr, sem_butex_waiter, &args));
+ ASSERT_EQ(0, pthread_create(
+ &pthreads[i], nullptr, sem_butex_waiter, &args));
+ }
+ EXPECT_EQ(0, ready.timed_wait(butil::seconds_from_now(10)));
+
+ EXPECT_EQ(0, batch ? bthread_sem_post_n(&sem, num)
+ : bthread_sem_post(&sem));
+ auto value = reinterpret_cast<butil::atomic<unsigned>*>(sem.butex);
+ EXPECT_EQ(initial_value + num,
value->load(butil::memory_order_relaxed));
+ // Also change the value for post_n(0), so late arrivals cannot enqueue
+ // after cleanup and block forever.
+ value->fetch_add(1, butil::memory_order_relaxed);
+ const int remaining = bthread::butex_wake_all(sem.butex);
+ for (int i = 0; i < N; ++i) {
+ EXPECT_EQ(0, bthread_join(bthreads[i], nullptr));
+ EXPECT_EQ(0, pthread_join(pthreads[i], nullptr));
+ }
Review Comment:
A queued pthread can be woken before entering `futex_wait_private`, causing
`wait_pthread` to return `EWOULDBLOCK` even though the wake operation counted
it (`src/bthread/butex.cpp:160–165`). Excluding that waiter from `nwoken` can
make the assertion at lines 196–197 fail with a correct semaphore
implementation, and retries do not clear the failed expectation. Use
bthread-only waiters for this wake-count assertion, or add synchronization that
distinguishes queued pthread wakeups from late arrivals.
--
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]
