chenBright commented on PR #3417:
URL: https://github.com/apache/brpc/pull/3417#issuecomment-5312611520
@walterzhaoJR On your two questions:
1. I'd argue no. A metric getter is invoked while a global VarMap shard lock
is held,
so it should be treated as a short, non-yieldable critical section — the
same
contract as any other "don't block/yield while holding a hot global
lock" path.
We don't need to make yielding a supported behavior; we need to make
sure an
accidental yield inside a getter cannot starve workers.
2. With the approach below this is preserved for free, without lifetime
pinning: `describe()`
still runs under the shard lock, and the getter neither yields nor
migrates, so the `Variable`
cannot be hidden or destroyed mid-callback (`hide()` would have to
acquire the very same
shard lock the callback is holding).
Proposed minimal fix:
This is a much smaller fix than either the recursive-mutex change or the
scheduler/bvar split.
Add two small pieces:
1. Mark the callback as a non-yieldable region. Wrap the `describe()` call
in `describe_exposed()`
with an RAII guard backed by a thread-local counter. A counter (not a
bool) is required because
the VarMap lock is `PTHREAD_MUTEX_RECURSIVE`, so a getter may
legitimately re-enter
`describe_exposed()`, and a plain set/reset bool would clear the flag on
the inner exit and leave
the rest of the outer getter unprotected:
```cpp
static thread_local int tls_in_metric_cb = 0;
struct ScopedNonYieldable {
ScopedNonYieldable() { ++tls_in_metric_cb; }
~ScopedNonYieldable() { --tls_in_metric_cb; }
};
```
2. Make the bthread suspension primitives honor the flag. At the existing
decision points that already
choose "bthread path vs pthread path" via `is_current_pthread_task()`,
add the flag as one more
condition, so that while the flag is set a contended wait blocks the
current worker (pthread semantics)
instead of calling `TaskGroup::sched()`:
```cpp
should_block_as_pthread(g) = (g == NULL) || g->is_current_pthread_task() ||
tls_in_metric_cb > 0;
```
This covers `butex_wait` (and therefore
`bthread::Mutex/cond/semaphore/rwlock`), plus `bthread_usleep` /
`bthread_yield`.
Trade-off, stated honestly:
This doesn't make getters safe to block arbitrarily. If a getter waits on a
bthread mutex whose holder
needs to be scheduled, it can still block that one worker under pthread
semantics. But it downgrades
the failure mode from "silent scheduler re-entrancy / UB in release builds"
to a well-defined,
documentable pthread-blocking constraint.
It's a small, localized change and doesn't require the scheduler/bvar core
split as a prerequisite.
--
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]
