wasphin commented on code in PR #3545:
URL: https://github.com/apache/brpc/pull/3545#discussion_r4053417067
##########
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());
Review Comment:
Please make this and the following `wait_started` checks non-fatal so the
explicit `stop_and_join()` is always reached. `timer_thread` was declared
before the `TimeKeeper` objects, so an early return destroys the callback
arguments first and only then invokes `TimerThread::~TimerThread()`. A callback
may start or still access a keeper during that window. `EXPECT_TRUE` preserves
the failure while ensuring the timer thread is joined before the keepers are
destroyed.
--
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]
