Worker Pool 防止饿死#51
Conversation
owent
left a comment
There was a problem hiding this comment.
AICR problem for src/atframe/modules/worker_pool_module.cpp:282
| worker_tick_action_container_type tick_handles_; | ||
| std::recursive_mutex tick_handle_lock_; | ||
| std::atomic<std::chrono::microseconds::rep> current_tick_interval_us_; | ||
| std::chrono::microseconds::rep current_tick_second_busy_us_; |
There was a problem hiding this comment.
MEDIUM
·
correctness
新增成员变量 current_tick_second_busy_us_ 和 current_tick_second_waited_us_ 使用普通标量类型,而同类中类似用途的变量 cpu_time_busy_us_ 和 cpu_time_sleep_us_ 使用 std::atomic 并通过 fetch_add 更新(见第473、491行)。如果这些新变量预期会被其他线程读取(例如用于诊断或监控),则存在数据竞争风险。建议:若需跨线程访问,应声明为 std::atomic 类型;若仅在工作线程内部访问,建议添加注释说明设计意图。
Location: src/atframe/modules/worker_pool_module.cpp:282-283
Referenced code (src/atframe/modules/worker_pool_module.cpp:282-283):
std::recursive_mutex tick_handle_lock_;
std::atomic<std::chrono::microseconds::rep> current_tick_interval_us_;
std::chrono::microseconds::rep current_tick_second_busy_us_;
std::chrono::microseconds::rep current_tick_second_waited_us_;
std::atomic<std::chrono::microseconds::rep> cpu_time_busy_us_;
AI Code Review
AI Code Review Summaryworker_pool_module.cpp 时序追踪问题Target: Worker Pool 防止饿死 审查范围审查了 发现问题
背景新增的 Problems (1)
Code reference:
|
There was a problem hiding this comment.
Pull request overview
This PR adds worker-pool throttling intended to prevent starvation by tracking per-worker busy/wait time and inserting a preserved sleep window once accumulated tick time reaches one second.
Changes:
- Adds per-worker counters for current-second busy and waited microseconds.
- Adds a worker-set preserve interval defaulting to 8000µs.
- Inserts an extra condition-variable wait after each accumulated second of worker activity.
Comments suppressed due to low confidence (1)
src/atframe/modules/worker_pool_module.cpp:481
- This added line exceeds the repository's
.clang-formatColumnLimit: 120; please wrap it or run clang-format to keep formatting consistent.
auto wait_preserve_us = self->get_owner().configure_tick_preserve_microseconds_in_second.load(std::memory_order_acquire);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self->current_tick_second_busy_us_ += std::chrono::duration_cast<std::chrono::microseconds>(busy_end_time - start_time).count(); | ||
| if (busy_end_time - start_time < tick_interval) { | ||
| std::unique_lock<std::mutex> lk_cv(self->waker_lock_); | ||
| self->current_tick_second_waited_us_ += std::chrono::duration_cast<std::chrono::microseconds>(tick_interval - (busy_end_time - start_time)).count(); |
| auto wait_preserve_us = self->get_owner().configure_tick_preserve_microseconds_in_second.load(std::memory_order_acquire); | ||
| if (wait_preserve_us <= 0) { | ||
| wait_preserve_us = 8000; | ||
| } | ||
|
|
||
| std::unique_lock<std::mutex> lk_cv(self->waker_lock_); | ||
| self->status_.store(static_cast<uint8_t>(worker_status::kSleeping), std::memory_order_release); | ||
| self->waker_cv_.wait_for(lk_cv, std::chrono::microseconds(wait_preserve_us)); | ||
| self->status_.store(static_cast<uint8_t>(worker_status::kRunning), std::memory_order_release); | ||
|
|
||
| self->cpu_time_sleep_us_.fetch_add(wait_preserve_us, std::memory_order_release); |
| if (self->current_tick_second_busy_us_ + self->current_tick_second_waited_us_ >= 1000000) { | ||
| self->current_tick_second_busy_us_ = 0; | ||
| self->current_tick_second_waited_us_ = 0; | ||
|
|
||
| auto wait_preserve_us = self->get_owner().configure_tick_preserve_microseconds_in_second.load(std::memory_order_acquire); |
| self->current_tick_second_busy_us_ += std::chrono::duration_cast<std::chrono::microseconds>(busy_end_time - start_time).count(); | ||
| if (busy_end_time - start_time < tick_interval) { | ||
| std::unique_lock<std::mutex> lk_cv(self->waker_lock_); | ||
| self->current_tick_second_waited_us_ += std::chrono::duration_cast<std::chrono::microseconds>(tick_interval - (busy_end_time - start_time)).count(); |
owent
left a comment
There was a problem hiding this comment.
AICR problem for src/atframe/modules/worker_pool_module.cpp:491
| self->waker_cv_.wait_for(lk_cv, std::chrono::microseconds(wait_preserve_us)); | ||
| self->status_.store(static_cast<uint8_t>(worker_status::kRunning), std::memory_order_release); | ||
|
|
||
| self->cpu_time_sleep_us_.fetch_add(wait_preserve_us, std::memory_order_release); |
There was a problem hiding this comment.
MEDIUM
·
correctness
cpu_time_sleep_us_ 的统计方式不一致:普通 tick 等待使用实际测量的睡眠时间(第 469-473 行),但新增的 preserve sleep 直接使用计划时间 wait_preserve_us(第 491 行)。如果 preserve sleep 因虚假唤醒或 waker_cv_ 被通知而提前结束,cpu_time_sleep_us_ 会高估实际睡眠时间。建议在第 489 行后添加类似第 469-473 行的实际时间测量逻辑。
Location: src/atframe/modules/worker_pool_module.cpp:491
Referenced code (src/atframe/modules/worker_pool_module.cpp:491):
self->status_.store(static_cast<uint8_t>(worker_status::kRunning), std::memory_order_release);
self->cpu_time_sleep_us_.fetch_add(wait_preserve_us, std::memory_order_release);
}
}
owent
left a comment
There was a problem hiding this comment.
AICR problem for src/atframe/modules/worker_pool_module.cpp:463
| self->current_tick_second_busy_us_ += std::chrono::duration_cast<std::chrono::microseconds>(busy_end_time - start_time).count(); | ||
| if (busy_end_time - start_time < tick_interval) { | ||
| std::unique_lock<std::mutex> lk_cv(self->waker_lock_); | ||
| self->current_tick_second_waited_us_ += std::chrono::duration_cast<std::chrono::microseconds>(tick_interval - (busy_end_time - start_time)).count(); |
There was a problem hiding this comment.
MEDIUM
·
correctness
current_tick_second_waited_us_ 在第463行累加的是预期等待时间而非实际等待时间。当条件变量在第466行被提前唤醒(spurious wakeup 或通知)时,计数器会高估实际经过的墙钟时间,导致第477行的检查条件过早触发 preserve sleep。例如:预期等待500ms但实际只等待了100ms时,计数器仍增加500ms,与 busy 时间求和后可能远超1秒墙钟时间。
Location: src/atframe/modules/worker_pool_module.cpp:463
Referenced code (src/atframe/modules/worker_pool_module.cpp:463):
if (busy_end_time - start_time < tick_interval) {
std::unique_lock<std::mutex> lk_cv(self->waker_lock_);
self->current_tick_second_waited_us_ += std::chrono::duration_cast<std::chrono::microseconds>(tick_interval - (busy_end_time - start_time)).count();
self->status_.store(static_cast<uint8_t>(worker_status::kSleeping), std::memory_order_release);Suggested fix:
考虑在 wait_for 返回后测量实际等待时间并累加,或使用墙钟时间戳来判断是否已过一秒。
No description provided.