Copilot commented on code in PR #3423:
URL: https://github.com/apache/brpc/pull/3423#discussion_r3698363565
##########
test/bthread_priority_queue_unittest.cpp:
##########
@@ -147,10 +147,16 @@ TEST_F(PriorityQueueTest,
start_foreground_priority_to_run) {
bthread::TaskGroup::address_meta(bthread_self());
meta->priority_index = 0;
+ std::vector<bthread_t> children;
+ children.reserve(ea->n_tasks);
for (int i = 0; i < ea->n_tasks; ++i) {
TaskArg* ta = new TaskArg{i};
bthread_t child;
bthread_start_urgent(&child, NULL, priority_task_fn, ta);
+ children.push_back(child);
+ }
Review Comment:
bthread_start_urgent() return value is not checked, but the new code
unconditionally stores the (potentially uninitialized) child tid and later
bthread_join()s it. If bthread_start_urgent fails, this can turn into undefined
behavior / spurious join errors and also leaks TaskArg.
##########
src/bthread/task_group.cpp:
##########
@@ -1018,8 +1018,10 @@ void TaskGroup::priority_to_run(void* args_in) {
if (args->meta->priority_index < 0) {
return g->push_rq(args->meta->tid);
}
- return g->control()->push_ed_priority_queue(
+ g->control()->push_ed_priority_queue(
args->tag, args->meta->priority_index, args->meta->tid);
+
+ g->control()->signal_task(1, args->tag);
Review Comment:
TaskGroup::priority_to_run() now calls TaskControl::signal_task() directly,
but this bypasses TaskGroup’s _nsignaled/_remote_nsignaled accounting that
backs TaskControl::get_cumulated_signal_count(). As a result, bvar-reported
signal counts will under-report once priority-queue wakeups happen via this
path.
--
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]
