Copilot commented on code in PR #3545:
URL: https://github.com/apache/brpc/pull/3545#discussion_r4026191450
##########
test/bthread_timer_thread_unittest.cpp:
##########
@@ -98,59 +114,70 @@ class TimeKeeper {
keeper->run();
}
+ bool wait_finished() {
+ return WaitUntil([this] {
+ return _finished.load(butil::memory_order_acquire);
+ });
+ }
+
+ bool wait_started() {
+ return WaitUntil([this] {
+ return _started.load(butil::memory_order_acquire);
+ });
+ }
+
timespec _expect_run_time;
bthread::TimerThread::TaskId _task_id;
private:
const char* _name;
- int _sleep_ms;
+ butil::atomic<int> _sleep_ms;
+ butil::atomic<bool> _started{false};
+ butil::atomic<bool> _finished{false};
std::vector<timespec> _run_times;
};
TEST(TimerThreadTest, RunTasks) {
bthread::TimerThread timer_thread;
ASSERT_EQ(0, timer_thread.start(nullptr));
- timespec _2s_later = butil::seconds_from_now(2);
+ timespec _2s_later = butil::milliseconds_from_now(20);
TimeKeeper keeper1(_2s_later, "keeper1");
keeper1.schedule(&timer_thread);
- TimeKeeper keeper2(_2s_later, "keeper2"); // same time with keeper1
+ TimeKeeper keeper2(butil::seconds_from_now(3600), "keeper2");
keeper2.schedule(&timer_thread);
- timespec _1s_later = butil::seconds_from_now(1);
+ timespec _1s_later = butil::milliseconds_from_now(10);
TimeKeeper keeper3(_1s_later, "keeper3");
keeper3.schedule(&timer_thread);
- timespec _10s_later = butil::seconds_from_now(10);
+ timespec _10s_later = butil::seconds_from_now(3600);
TimeKeeper keeper4(_10s_later, "keeper4");
keeper4.schedule(&timer_thread);
TimeKeeper keeper5(_10s_later, "keeper5");
keeper5.schedule(&timer_thread);
- // sleep 1 second, and unschedule task2
- LOG(INFO) << "Sleep 1s";
- sleep(1);
- timer_thread.unschedule(keeper2._task_id);
- timer_thread.unschedule(keeper4._task_id);
+ ASSERT_EQ(0, timer_thread.unschedule(keeper2._task_id));
+ ASSERT_EQ(0, timer_thread.unschedule(keeper4._task_id));
timespec old_time = { 0, 0 };
TimeKeeper keeper6(old_time, "keeper6");
+ timespec keeper6_addtime = butil::seconds_from_now(0);
keeper6.schedule(&timer_thread);
- const timespec keeper6_addtime = butil::seconds_from_now(0);
- // sleep 10 seconds and stop.
- LOG(INFO) << "Sleep 2s";
- sleep(2);
+ ASSERT_TRUE(keeper1.wait_started());
+ ASSERT_TRUE(keeper3.wait_started());
+ ASSERT_TRUE(keeper6.wait_started());
Review Comment:
These fatal assertions can return before `timer_thread.stop_and_join()`
runs. If a callback is preempted after starting but before publishing its
`_started` flag, the 10-second wait can fail while it still holds a pointer to
a stack `TimeKeeper`; destruction order destroys the keepers before
`TimerThread` joins the callback. Use non-fatal checks here (or an earlier
cleanup guard) so the timer is joined before the keepers leave scope.
##########
test/bthread_fd_unittest.cpp:
##########
@@ -411,39 +411,62 @@ TEST(FDTest, add_existing_fd) {
#endif
}
+struct EpollWaitArg {
+ int epfd;
+ butil::atomic<bool> done{false};
+ int result = 0;
+ int error = 0;
+};
+
void* epoll_waiter(void* arg) {
+ EpollWaitArg* a = static_cast<EpollWaitArg*>(arg);
#if defined(OS_LINUX)
epoll_event e;
- if (1 == epoll_wait((int)(intptr_t)arg, &e, 1, -1)) {
- std::cout << e.events << std::endl;
- }
+ a->result = epoll_wait(a->epfd, &e, 1, 10000);
#elif defined(OS_MACOSX)
struct kevent e;
- if (1 == kevent((int)(intptr_t)arg, nullptr, 0, &e, 1, nullptr)) {
- std::cout << e.flags << std::endl;
- }
+ timespec timeout = {10, 0};
+ a->result = kevent(a->epfd, nullptr, 0, &e, 1, &timeout);
#endif
- std::cout << pthread_self() << " quits" << std::endl;
+ a->error = errno;
+ a->done.store(true, butil::memory_order_release);
return nullptr;
}
TEST(FDTest, interrupt_pthread) {
#if defined(OS_LINUX)
- const int epfd = epoll_create(1024);
+ butil::fd_guard epfd(epoll_create(1024));
#elif defined(OS_MACOSX)
- const int epfd = kqueue();
+ butil::fd_guard epfd(kqueue());
#endif
- pthread_t th, th2;
- ASSERT_EQ(0, pthread_create(&th, nullptr, epoll_waiter,
(void*)(intptr_t)epfd));
- ASSERT_EQ(0, pthread_create(&th2, nullptr, epoll_waiter,
(void*)(intptr_t)epfd));
- bthread_usleep(100000L);
- std::cout << "wake up " << th << std::endl;
- bthread::interrupt_pthread(th);
- bthread_usleep(100000L);
- std::cout << "wake up " << th2 << std::endl;
- bthread::interrupt_pthread(th2);
- pthread_join(th, nullptr);
- pthread_join(th2, nullptr);
+ ASSERT_GE(epfd, 0);
+ EpollWaitArg args[2];
+ pthread_t threads[2];
+ size_t started = 0;
+ for (; started < ARRAY_SIZE(threads); ++started) {
+ args[started].epfd = epfd;
+ int rc = pthread_create(&threads[started], nullptr,
+ epoll_waiter, &args[started]);
+ EXPECT_EQ(0, rc);
+ if (rc != 0) {
+ break;
+ }
+ }
+ int64_t deadline = butil::cpuwide_time_us() + 15000000L;
+ for (size_t i = 0; i < started; ++i) {
+ // Signals are not persistent. Retry until the syscall observes one;
+ // keep the pthread joinable until all signalling is finished.
+ while (!args[i].done.load(butil::memory_order_acquire) &&
+ butil::cpuwide_time_us() < deadline) {
+ EXPECT_EQ(0, bthread::interrupt_pthread(threads[i]));
+ bthread_usleep(1000);
Review Comment:
`done` is checked before this call, so the waiter can finish and exit in
that window; `pthread_kill` then returns `ESRCH` even though the interruption
succeeded by no longer being necessary, and this `EXPECT_EQ` records a flaky
failure. Treat `ESRCH` as the terminal completion race and let the final result
assertions validate why the waiter exited.
--
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]
