wasphin commented on code in PR #3545:
URL: https://github.com/apache/brpc/pull/3545#discussion_r4053422203
##########
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:
I agree that the remaining assertions are not meaningful after this timeout,
but the concern is not merely a messy teardown. A fatal assertion starts stack
unwinding immediately, and `timer_thread` was constructed before the
`TimeKeeper` objects, so the keepers are destroyed before
`TimerThread::~TimerThread()` joins the callback. A callback preempted before
publishing `_started` can therefore resume with a dangling pointer. We can
preserve fail-fast behavior by calling `timer_thread.stop_and_join()` first and
then using `FAIL()`/`ASSERT_TRUE`, rather than continuing the rest of the test.
--
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]
