Copilot commented on code in PR #3633:
URL: https://github.com/apache/brpc/pull/3633#discussion_r4336688130
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type ==
BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
Review Comment:
`_rq.push()` appears to be a fallible operation (it’s checked elsewhere in
this function). Ignoring the return value here risks silently dropping
`run_tid` if the push fails (e.g., due to unexpected queue state or
implementation changes). Prefer checking the return and handling failure
explicitly (retry, assert with a clear message, or take an alternate path) to
avoid task loss.
##########
src/bthread/task_group_inl.h:
##########
@@ -93,11 +93,65 @@ inline void TaskGroup::push_rq(bthread_t tid) {
// brpc)
flush_nosignal_tasks();
LOG_EVERY_SECOND(ERROR) << "_rq is full, capacity=" << _rq.capacity();
- // TODO(gejun): May cause deadlock when all workers are spinning here.
- // A better solution is to pop and run existing bthreads, however which
- // make set_remained()-callbacks do context switches and need extensive
- // reviews on related code.
- ::usleep(1000);
+ if (is_current_pthread_task() && !is_current_main_task()) {
+ // A pthread-mode bthread runs on _main_stack and cannot be
+ // scheduled out, fall back to waiting.
+ ::usleep(1000);
+ continue;
+ }
+ bthread_t run_tid = 0;
+#ifndef BTHREAD_FAIR_WSQ
+ const bool popped = _rq.pop(&run_tid);
+#else
+ const bool popped = _rq.steal(&run_tid);
+#endif
+ if (!popped) {
+ // The queue was drained by stealers between the failed push and
+ // the pop, retrying the push will succeed.
+ continue;
+ }
+ if (is_current_main_task() &&
+ address_meta(run_tid)->attr.stack_type ==
BTHREAD_STACKTYPE_PTHREAD) {
+ // pthread-mode tasks are run inline on _main_stack by the
+ // dispatch loops (run_main_task/task_runner). Switching to one
+ // from the main task itself causes no stack jump and would
+ // silently drop it, put it back and wait; stealers may run it
+ // on other workers.
+ _rq.push(run_tid);
+ ::usleep(1000);
+ continue;
+ }
Review Comment:
This branch makes no progress toward freeing space for the original `tid`
enqueue: it pops one item and immediately pushes it back, leaving `_rq` full
again, then sleeps. If stealers aren’t draining (the exact failure mode this PR
targets), `push_rq()` can spin/sleep indefinitely here. A concrete fix is to
use the freed slot to enqueue `tid` first (so the caller can make progress),
and only then handle `run_tid` (e.g., keep `run_tid` locally and retry
re-queueing it later, or search for a non-pthread task to run inline instead of
immediately pushing the pthread-mode one back).
--
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]
