wasphin commented on code in PR #3545:
URL: https://github.com/apache/brpc/pull/3545#discussion_r4053404934
##########
test/bthread_fd_unittest.cpp:
##########
@@ -484,49 +514,73 @@ TEST(FDTest, invalid_epoll_events) {
ASSERT_EQ(0, bthread_fd_wait(fds[0], EVFILT_READ));
#endif
tm.stop();
- ASSERT_LT(tm.m_elapsed(), 20);
+ // Successful readiness, not scheduler latency, is the contract.
ASSERT_EQ(0, bthread_join(th, nullptr));
ASSERT_EQ(0, bthread_close(fds[0]));
}
+struct FDWaitArg {
+ int fd;
+ int timeout_ms;
+ int result = 0;
+ int error = 0;
+};
+
void* wait_for_the_fd(void* arg) {
- timespec ts = butil::milliseconds_from_now(50);
+ FDWaitArg* a = static_cast<FDWaitArg*>(arg);
+ timespec ts = butil::milliseconds_from_now(a->timeout_ms);
#if defined(OS_LINUX)
- bthread_fd_timedwait(*(int*)arg, EPOLLIN, &ts);
+ a->result = bthread_fd_timedwait(a->fd, EPOLLIN, &ts);
#elif defined(OS_MACOSX)
- bthread_fd_timedwait(*(int*)arg, EVFILT_READ, &ts);
+ a->result = bthread_fd_timedwait(a->fd, EVFILT_READ, &ts);
#endif
+ a->error = errno;
+ if (a->result == -1 && a->error == ETIMEDOUT) {
+ EXPECT_GE(butil::gettimeofday_us(),
butil::timespec_to_microseconds(ts));
+ }
return nullptr;
}
TEST(FDTest, timeout) {
int fds[2];
ASSERT_EQ(0, pipe(fds));
+ FDWaitArg args[2];
+ for (auto& arg : args) {
+ arg.fd = fds[0];
+ arg.timeout_ms = 50;
+ }
pthread_t th;
- ASSERT_EQ(0, pthread_create(&th, nullptr, wait_for_the_fd, &fds[0]));
+ ASSERT_EQ(0, pthread_create(&th, nullptr, wait_for_the_fd, &args[0]));
bthread_t bth;
- ASSERT_EQ(0, bthread_start_urgent(&bth, nullptr, wait_for_the_fd,
&fds[0]));
- butil::Timer tm;
- tm.start();
+ ASSERT_EQ(0, bthread_start_urgent(&bth, nullptr, wait_for_the_fd,
&args[1]));
ASSERT_EQ(0, pthread_join(th, nullptr));
ASSERT_EQ(0, bthread_join(bth, nullptr));
- tm.stop();
- ASSERT_LT(tm.m_elapsed(), 80);
ASSERT_EQ(0, bthread_close(fds[0]));
ASSERT_EQ(0, bthread_close(fds[1]));
+ for (auto& arg : args) {
+ ASSERT_EQ(-1, arg.result);
+ ASSERT_EQ(ETIMEDOUT, arg.error);
+ }
}
TEST(FDTest, close_should_wakeup_waiter) {
int fds[2];
ASSERT_EQ(0, pipe(fds));
+ FDWaitArg arg;
+ arg.fd = fds[0];
+ arg.timeout_ms = 10000;
bthread_t bth;
- ASSERT_EQ(0, bthread_start_urgent(&bth, nullptr, wait_for_the_fd,
&fds[0]));
- butil::Timer tm;
- tm.start();
+ ASSERT_EQ(0, bthread_start_urgent(&bth, nullptr, wait_for_the_fd, &arg));
+ auto* meta = bthread::TaskGroup::address_meta(bth);
+ int64_t deadline = butil::cpuwide_time_us() + 5000000L;
+ while (meta->current_waiter.load(butil::memory_order_acquire) == nullptr &&
+ butil::cpuwide_time_us() < deadline) {
+ bthread_usleep(1000);
+ }
+ ASSERT_NE(nullptr, meta->current_waiter.load(butil::memory_order_acquire));
Review Comment:
Please make this observation non-fatal so cleanup remains reachable. If
registration is not observed before the deadline, `ASSERT_NE` skips
`bthread_close`, `bthread_join`, and closing the other pipe end. The worker may
still reference the stack-allocated `arg` after the test returns. Using
`EXPECT_NE` allows the close/join path to run and the later result assertion to
report any behavioral failure safely.
##########
test/bthread_mutex_unittest.cpp:
##########
@@ -52,8 +52,14 @@ TEST(MutexTest, sanity) {
ASSERT_EQ(1u, *get_butex(m));
bthread_t th1;
ASSERT_EQ(0, bthread_start_urgent(&th1, nullptr, locker, &m));
- usleep(5000); // wait for locker to run.
- ASSERT_EQ(257u, *get_butex(m)); // contention
+ auto* state = reinterpret_cast<butil::atomic<unsigned>*>(get_butex(m));
+ int64_t deadline = butil::cpuwide_time_us() + 5000000L;
+ while (state->load(butil::memory_order_relaxed) != 257u &&
+ butil::cpuwide_time_us() < deadline) {
+ usleep(1000);
+ }
+ // Keep cleanup reachable even if the worker did not run in time.
+ ASSERT_EQ(257u, state->load(butil::memory_order_relaxed));
Review Comment:
Please use `EXPECT_EQ` here instead of `ASSERT_EQ`. If the contended state
is not observed before the deadline, this fatal assertion returns from the test
with the mutex still locked and `th1` still blocked while referencing the
stack-allocated mutex. A non-fatal assertion lets the existing unlock, join,
and destroy cleanup run before the test reports the failure.
--
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]
