Copilot commented on code in PR #3384:
URL: https://github.com/apache/brpc/pull/3384#discussion_r3567849626
##########
test/bthread_timer_thread_unittest.cpp:
##########
@@ -254,4 +256,113 @@ TEST(TimerThreadTest, schedule_and_unschedule_in_task) {
keeper5.expect_first_run();
}
+static void noop_routine(void*) {}
+
+// Tasks that are unscheduled after being pulled into the timer thread's
+// internal heap must not stay there (occupying a pooled Task slot) until
+// their run_time. With large timeouts that would let memory grow ~ qps *
+// timeout. The timer thread should sweep them out and keep the heap bounded.
+TEST(TimerThreadTest, sweep_unscheduled_tasks_in_heap) {
+ // Lower the sweep threshold so the test doesn't need a huge heap.
+ GFLAGS_NAMESPACE::SetCommandLineOption(
+ "brpc_timer_heap_sweep_min_size", "512");
+
+ bthread::TimerThread timer_thread;
+ ASSERT_EQ(0, timer_thread.start(NULL));
+
+ // Run far enough in the future that these tasks never fire on their own.
+ const timespec far = butil::seconds_from_now(100000);
+ const size_t kBatch = 2000;
+ const size_t kRounds = 20;
+
+ int64_t max_pending = 0;
+ for (size_t r = 0; r < kRounds; ++r) {
+ std::vector<bthread::TimerThread::TaskId> ids;
+ ids.reserve(kBatch);
+ for (size_t i = 0; i < kBatch; ++i) {
+ ids.push_back(timer_thread.schedule(noop_routine, NULL, far));
+ }
+ // A near-term task forces the timer thread to wake up and consume the
+ // buckets, so the far tasks above land in the heap (alive).
+ timer_thread.schedule(noop_routine, NULL,
+ butil::milliseconds_from_now(1));
+ usleep(20000); // let the timer thread consume the buckets
+
+ // Now unschedule the far tasks: they become dead-in-heap, exactly the
+ // case that used to linger until run_time.
+ for (size_t i = 0; i < ids.size(); ++i) {
+ timer_thread.unschedule(ids[i]);
+ }
+ // Another near-term task wakes the timer thread again, triggering the
+ // sweep that reclaims the dead tasks.
+ timer_thread.schedule(noop_routine, NULL,
+ butil::milliseconds_from_now(1));
+ usleep(20000);
+
+ const int64_t pending = timer_thread.pending_task_count();
+ LOG(INFO) << "round=" << r << " pending=" << pending;
+ max_pending = std::max(max_pending, pending);
+ }
+
+ // Without the sweep, pending would grow to ~ kBatch * kRounds (40000).
+ // With it, the heap is bounded by roughly a couple of sweep thresholds.
+ EXPECT_LT(max_pending, (int64_t)(kBatch * kRounds) / 4)
+ << "dead tasks are not being reclaimed from the heap";
+
+ timer_thread.stop_and_join();
+}
+
+// When every pending task is far in the future, the nearest run_time never
+// arrives, so the timer thread used to sleep for the whole duration. Newly
+// scheduled-then-unscheduled tasks would then pile up in the buckets (never
+// consumed, so never reclaimed) for that entire time. The capped wakeup makes
+// the timer thread wake up periodically to drain the buckets, keeping the pool
+// bounded regardless of how far the timeouts are.
+TEST(TimerThreadTest, periodic_wakeup_reclaims_bucket_tasks) {
+ // Wake up frequently and keep any heap slippage swept too.
+ GFLAGS_NAMESPACE::SetCommandLineOption(
+ "brpc_timer_max_wakeup_interval_ms", "50");
+ GFLAGS_NAMESPACE::SetCommandLineOption(
+ "brpc_timer_heap_sweep_min_size", "512");
Review Comment:
This test changes global gflags (brpc_timer_max_wakeup_interval_ms and
brpc_timer_heap_sweep_min_size) but never restores them, which can affect
subsequent tests. Save previous values and restore them on scope exit; also
check SetCommandLineOption return values so failures to set the flag don’t turn
into confusing test flakes.
##########
test/bthread_timer_thread_unittest.cpp:
##########
@@ -254,4 +256,113 @@ TEST(TimerThreadTest, schedule_and_unschedule_in_task) {
keeper5.expect_first_run();
}
+static void noop_routine(void*) {}
+
+// Tasks that are unscheduled after being pulled into the timer thread's
+// internal heap must not stay there (occupying a pooled Task slot) until
+// their run_time. With large timeouts that would let memory grow ~ qps *
+// timeout. The timer thread should sweep them out and keep the heap bounded.
+TEST(TimerThreadTest, sweep_unscheduled_tasks_in_heap) {
+ // Lower the sweep threshold so the test doesn't need a huge heap.
+ GFLAGS_NAMESPACE::SetCommandLineOption(
+ "brpc_timer_heap_sweep_min_size", "512");
Review Comment:
This test changes the global gflag brpc_timer_heap_sweep_min_size but never
restores it, which can leak configuration into later tests and cause
order-dependent failures. Save the previous value and restore it on scope exit;
also assert that SetCommandLineOption succeeded so the test doesn’t silently
run with defaults.
##########
src/bthread/timer_thread.cpp:
##########
@@ -408,6 +466,12 @@ void TimerThread::run() {
BT_VLOG << "pull again, tasks=" << tasks.size();
continue;
}
Review Comment:
pending_task_count() is updated only on the non-pull_again path. When
pull_again is taken, _npending can stay stale even though the heap content may
change across iterations, making the new observability API misleading (and
potentially flaky for tests that sample it). Update _npending before continuing.
--
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]
