Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a worker-pool event callback mechanism to libatapp so callers can observe worker lifecycle transitions (created/started/exiting/removed), and introduces a per-worker worker_unique_id to correlate events across worker ID reuse/moves.
Changes:
- Add
worker_unique_idtoworker_contextand propagate it through worker creation/selection paths. - Implement worker lifecycle event callback registration/removal APIs in
worker_pool_module, plus callback dispatch on lifecycle transitions. - Add a new unit test exercising event registration/removal behavior and lifecycle event delivery.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/case/atapp_worker_pool_test.cpp | Adds a test validating worker lifecycle event callbacks and handle-removal behaviors. |
| src/atframe/modules/worker_pool_module.cpp | Implements callback storage/dispatch, worker unique ID assignment, and lifecycle hooks firing callbacks. |
| include/atframe/modules/worker_pool_module.h | Exposes the new event callback registration/removal APIs. |
| include/atframe/modules/worker_context.h | Extends worker_context with worker_unique_id and introduces callback/handle types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1471
to
1475
| std::list<worker_event_callback_internal_data_pointer>* event_on_worker_created_ptr = nullptr; | ||
| std::list<worker_event_callback_internal_data_pointer> event_on_worker_created_data; | ||
|
|
||
| uint32_t expect_workers = worker_set_->current_expect_workers.load(std::memory_order_acquire); | ||
| std::lock_guard<std::recursive_mutex> lg{worker_set_->worker_lock}; |
Comment on lines
+143
to
+149
| static void internal_remove_event_callback(std::recursive_mutex& lock, | ||
| std::list<worker_event_callback_internal_data_pointer>& callback_list, | ||
| const worker_event_callback_handle_type& handle) { | ||
| // 加锁后再检查状态，handle->owner 可能被其他线程的 remove/cleanup 并发修改 | ||
| std::lock_guard<std::recursive_mutex> lg{lock}; | ||
| if (handle->owner != &callback_list) { | ||
| return; |
Comment on lines
19
to
+29
| struct UTIL_SYMBOL_VISIBLE worker_context { | ||
| // worker id 指示当前是第几个worker，0表示主线程，1表示第一个工作线程，依次类推。 | ||
| // worker id 可能被复用或转移工作线程，但同时每个 worker id 指向唯一一个线程 | ||
| uint32_t worker_id = 0; | ||
|
|
||
| inline worker_context() noexcept : worker_id(0) {} | ||
| explicit inline worker_context(uint32_t id) noexcept : worker_id(id) {} | ||
| // worker_unique_id 指示当前worker的唯一标识，不会随着线程转移而变化 | ||
| uint64_t worker_unique_id = 0; | ||
|
|
||
| inline worker_context() noexcept : worker_id(0), worker_unique_id(0) {} | ||
| explicit inline worker_context(uint32_t id, uint64_t unique_id = 0) noexcept | ||
| : worker_id(id), worker_unique_id(unique_id) {} |
AI Code Review
AI Code Review Summaryworker_pool: 新增事件回调与 worker_unique_id，foreach_worker_quickly 存在 stable 槽位丢失回归Target: Add worker pool event handle 审查范围PR #58 (owent/libatapp) 为 worker pool 新增四类事件回调（ 审查基于完整源码（已读取 问题汇总（1 项，medium）
已确认无问题的点
限制
Problems (1)
Code reference:
|
atframework-aicr
Bot
added
aicr
aicr:problem:medium
aicr:reviewed
aicr:problem:low
labels
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/atframe/modules/worker_pool_module.cpp:609
- User-provided worker_exiting callbacks are invoked on the worker thread without any exception guard. If exceptions are enabled and a callback throws, the exception will escape the thread entry function and trigger std::terminate.
for (auto& fn : event_on_worker_exiting) {
if (fn && fn->callback) {
fn->callback(self->get_context());
}
src/atframe/modules/worker_pool_module.cpp:150
- internal_remove_event_callback reads handle->owner while only holding the mutex for the target callback_list. Because each event type uses a different mutex, a mismatched remove (or cleanup) can concurrently write handle->owner under a different mutex, which is a data race even though the function returns early for mismatched lists.
// 加锁后再检查状态，handle->owner 可能被其他线程的 remove/cleanup 并发修改
std::lock_guard<std::recursive_mutex> lg{lock};
if (handle->owner != &callback_list) {
return;
}
include/atframe/modules/worker_context.h:30
- worker_context removed the previous single-argument constructor (worker_context(uint32_t id)), which is a source-level breaking change for existing callers that build contexts by worker_id only. Keeping a 1-arg overload that defaults worker_unique_id to 0 preserves backward compatibility while still supporting the new unique-id field.
inline worker_context() noexcept : worker_id(0), worker_unique_id(0) {}
explicit inline worker_context(uint32_t id, uint64_t unique_id) noexcept
: worker_id(id), worker_unique_id(unique_id) {}
Comment on lines
+482
to
+486
| for (auto& fn : event_on_worker_start) { | ||
| if (fn && fn->callback) { | ||
| fn->callback(self->get_context()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
include/atframe/modules/worker_context.h:21
- The comment about worker_id is a bit ambiguous/contradictory ("可能被复用或转移" vs "同时...指向唯一一个线程"). Clarify that uniqueness is at a given point in time to avoid confusing API consumers.
// worker id 指示当前是第几个worker，0表示主线程，1表示第一个工作线程，依次类推。
// worker id 可能被复用或转移工作线程，但同时每个 worker id 指向唯一一个线程
Comment on lines
+28
to
+30
| inline worker_context() noexcept : worker_id(0), worker_unique_id(0) {} | ||
| explicit inline worker_context(uint32_t id, uint64_t unique_id) noexcept | ||
| : worker_id(id), worker_unique_id(unique_id) {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.