Fix potential bvar deadlock by running describe()/dump() outside the global VarMap lock

[chenBright]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

bvar's global VarMap is guarded by a pthread mutex. Variable::describe_exposed()
(and describe_series_exposed(), dump_exposed(), plus the multi-dimension
MVariableBase counterparts) used to invoke var->describe() while holding that lock.
For PassiveStatus, describe() runs a user-provided callback; if the callback yields
the bthread (e.g. by acquiring a bthread::Mutex), the pthread mutex is never released
and the process deadlocks.

What is changed and the side effects?

Changed:

Run user callbacks OUTSIDE the global map lock via a small indirection handle:

• New bvar/detail/exposed_ref.h: ExposedRef<T> (a reference-counted handle
  guarding an exposed object). It uses butil::Mutex + butil::ConditionVariable.
• describe_exposed() / describe_series_exposed() / get_exposed(): under the
  map lock they now only seek + acquire() (ref-count +1, serialized with hide()'s
  erase); the lock is released, describe() is called outside the lock, then release().
• hide() now also invalidates the handle and blocks (hide_and_wait()) until
  all in-flight readers finish, so a Variable cannot be destroyed while a concurrent
  describe() is still using it. Each expose() rebuilds a fresh handle (the old one
  is single-use once hidden).
• MVariableBase::describe_exposed() / dump_exposed() get the same treatment;
  dump() in particular is moved outside the lock because Dumper is a
  user-overridable interface that may yield.
• Since callbacks no longer run under the lock, the recursive VarMap mutex is no
  longer needed and is reverted to a plain mutex (now consistent with MVarMap).

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

Pull request overview

This PR addresses a deadlock risk in bvar’s global exposed-variable maps by ensuring user-overridable callbacks (e.g., PassiveStatus::describe() and Dumper::dump()) run outside the global VarMap/MVarMap pthread mutex, while still preventing use-after-free during concurrent hide()/destruction.

Changes:

• Introduces bvar/detail/exposed_ref.h (detail::ExposedRef<T>) as a reference-counted indirection handle so readers can safely call describe()/dump() after releasing the global map lock.
• Refactors Variable::{describe_exposed, describe_series_exposed, get_exposed} and MVariableBase::{describe_exposed, dump_exposed} to acquire/release via ExposedRef and move callback execution outside the global lock; hide() now waits for in-flight readers.
• Adds regression tests covering (1) destructor waiting for in-flight describe_exposed() and (2) reproducing the bthread-yield deadlock scenario (issue apache#2888) without hanging.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/bvar_variable_unittest.cpp	Adds a test ensuring a Variable’s destruction blocks until an in-flight describe_exposed() finishes.
test/bthread_unittest.cpp	Adds a regression test reproducing the original deadlock pattern (many bthreads + yielding callback) and validating it no longer hangs.
src/bvar/variable.h	Adds SharedExposedRef and stores an indirection handle in Variable to support lock-free callback execution.
src/bvar/variable.cpp	Switches VarEntry to store SharedExposedRef, moves describe*/get_exposed work outside the global lock, and makes hide() wait for in-flight readers.
src/bvar/mvariable.h	Adds SharedExposedRef and stores an indirection handle in MVariableBase for the same pattern as Variable.
src/bvar/mvariable.cpp	Refactors exposed lookups and dump_exposed() to acquire handles under lock, then call describe()/dump() outside the lock; hide() waits for readers.
src/bvar/detail/exposed_ref.h	New ref-counted handle providing acquire()/release()/hide_and_wait() coordination between readers and hide().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/bvar/mvariable.h:123

• Adding SharedExposedRef _ref as a data member changes the size/layout of the public bvar::MVariableBase class, which can break ABI for existing binaries when brpc is linked dynamically. Consider storing the ExposedRef handle only in the global MVarMap entry and retrieving it during hide(), or otherwise documenting the ABI impact.

protected:
    std::string _name;
    // Shared indirection handle for describe()/dump() outside the MVarMap lock.
    SharedExposedRef _ref;
};

src/bvar/variable.h:248

• Adding SharedExposedRef _ref as a data member changes the size/layout of the public bvar::Variable base class. If brpc is used as a shared library, this is an ABI break for existing binaries that define Variable subclasses. Consider keeping the ExposedRef only in the global map entry (capture a local copy of the handle in hide() before erasing) or explicitly documenting the ABI break in release notes/PR description.

    std::string _name;
    // Shared indirection handle for calling describe() outside the VarMap lock.
    SharedExposedRef _ref;
};

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

test/bvar_variable_unittest.cpp:442

• The wait for ctx.entered is an unbounded busy-loop; if describe_exposed() fails (e.g. name not found) this test will hang indefinitely and leave describer joinable (leading to std::terminate on failure paths). Add a bounded wait with a clean failure path that releases the callback and joins the thread.

    while (!ctx.entered.load()) {
        usleep(1000);
    }

test/bvar_variable_unittest.cpp:456

• Using ASSERT_FALSE here can prematurely abort the test and skip the cleanup/join logic below, which can trigger std::terminate due to still-joinable threads. Prefer EXPECT_FALSE so the test can still release the callback and join both threads before returning.

    ASSERT_FALSE(destructed.load())
        << "Variable dtor did not wait for the in-flight describe() to finish.";

test/bthread_unittest.cpp:742

• describe_same_var ignores the return code of describe_exposed(), so this test can "pass" even if the variable was never exposed (all calls return -1 quickly). Record failures and assert after joins so the test validates progress for the correct reason.

TEST_F(BthreadTest, describe_exposed_yields_in_bthread_no_deadlock) {
    bvar::PassiveStatus<int> ps(
        "bthread_describe_deadlock", yielding_getfn, nullptr);

    // n >> bthread_concurrency, so that in the buggy version every worker ends

src/bvar/mvariable.cpp:192

• On name conflict, expose_impl() clears _name (so the mvariable is NOT exposed) but still returns 0 a few lines below. This contradicts the header contract ("Return 0 on success, -1 otherwise") and can mislead callers into thinking the variable was exposed.

        MVarEntry* entry = m.seek(_name);
        if (entry == nullptr) {
            entry = &m[_name];
            entry->ref = _ref;
            return 0;