wwbmmm commented on code in PR #3551:
URL: https://github.com/apache/brpc/pull/3551#discussion_r4059768262
##########
src/bthread/butex.cpp:
##########
@@ -573,26 +573,37 @@ void wait_for_butex(void* arg) {
BAIDU_SCOPED_LOCK(b->waiter_lock);
if (b->value.load(butil::memory_order_relaxed) != bw->expected_value) {
bw->waiter_state = WAITER_STATE_UNMATCHEDVALUE;
- } else if (bw->waiter_state == WAITER_STATE_READY/*1*/ &&
- !bw->task_meta->interrupted) {
- if (args->prepend) {
- b->waiters.Prepend(bw);
- } else {
- b->waiters.Append(bw);
- }
- bw->container.store(b, butil::memory_order_relaxed);
+ } else {
+ // Checking `interrupted` and publishing `bw->container` must be
+ // atomic with respect to TaskGroup::interrupt(), which sets
+ // `interrupted` and consumes `current_waiter' under the same
+ // `version_lock`. Otherwise interrupt() may consume `bw` in
between
+ // and its erase_from_butex() does nothing because `container` is
+ // still nullptr, leaving this bthread queued but never woken up.
+ // `container` cannot be published upfront: it must stay nullptr
until
+ // the bthread is off its stack, see the comment after this block.
+ BAIDU_SCOPED_LOCK(bw->task_meta->version_lock);
+ if (bw->waiter_state == WAITER_STATE_READY/*1*/ &&
+ !bw->task_meta->interrupted) {
+ if (args->prepend) {
+ b->waiters.Prepend(bw);
+ } else {
+ b->waiters.Append(bw);
+ }
+ bw->container.store(b, butil::memory_order_relaxed);
#ifdef BRPC_BTHREAD_TRACER
- bw->control->_task_tracer.set_status(TASK_STATUS_SUSPENDED,
bw->task_meta);
+ TaskTracer::set_status_unsafe(TASK_STATUS_SUSPENDED,
bw->task_meta);
#endif // BRPC_BTHREAD_TRACER
- if (bw->abstime != nullptr) {
- bw->sleep_id = get_global_timer_thread()->schedule(
- erase_from_butex_and_wakeup, bw, *bw->abstime);
- if (!bw->sleep_id) { // TimerThread stopped.
- errno = ESTOP;
- erase_from_butex_and_wakeup(bw);
+ if (bw->abstime != nullptr) {
+ bw->sleep_id = get_global_timer_thread()->schedule(
+ erase_from_butex_and_wakeup, bw, *bw->abstime);
+ if (!bw->sleep_id) { // TimerThread stopped.
+ errno = ESTOP;
+ erase_from_butex_and_wakeup(bw);
Review Comment:
[replied by brpc-oncall robot] `erase_from_butex_and_wakeup()` is now called
while `bw->task_meta->version_lock` is held (taken at line 585). For a bthread
waiter it ends up in `TaskGroup::ready_to_run{,_remote}()` ->
`TaskTracer::set_status(TASK_STATUS_READY, bw->task_meta)`, which acquires the
very same `version_lock` (`TaskMeta::version_lock` is a non-recursive
`pthread_spinlock_t`). So in a `WITH_BTHREAD_TRACER` build this self-deadlocks
whenever `TimerThread::schedule()` fails. Please keep only the `interrupted`
check, the enqueue and the `container` store inside the `version_lock` scope,
and move the tracer update / timer scheduling (and this erase-and-wakeup) out
of it.
--
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]
