Fix bvar worker starvation with recursive bthread mutex

[walterzhaoJR]

Replace the recursive pthread mutex protecting bvar's VarMap with a bthread-aware recursive mutex so yielding metric callbacks do not block worker pthreads. Add regression coverage for worker starvation, recursive ownership across bthread migration, pthread use, and high contention.

Related issue: #2888

What problem does this PR solve?

Issue Number: resolves #2888

Problem Summary:

bvar::Variable protects its global VarMap with a recursive pthread mutex. If a metric callback, such as a PassiveStatus getter, yields while holding this lock, other bthreads may block their worker pthreads while waiting for it. Once all workers are blocked, the lock holder cannot resume, causing worker starvation or deadlock.

A pthread recursive mutex also identifies its owner by OS pthread, which does not correctly represent a bthread that may migrate between workers.

What is changed and the side effects?

Changed:

• Add bthread_recursive_mutex_t and its C++ wrapper, bthread::RecursiveMutex.
• Track ownership by bthread ID for bthreads while retaining support for native pthread callers.
• Use bthread_mutex_t for contention so waiting bthreads are suspended instead of blocking worker pthreads.
• Replace the recursive pthread mutex protecting bvar's VarMap with bthread::RecursiveMutex.
• Add regression tests covering:
  • bvar worker starvation;
  • recursive ownership across bthread yields and worker migration;
  • native pthread and C++ wrapper usage;
  • high-contention recursive locking.
• Keep the standalone test_bvar target independent of the full bthread runtime.

Side effects:

• Performance effects:

  • Recursive lock operations add small owner and recursion-depth checks.
  • Under contention, logical bthreads are suspended instead of blocking worker pthreads.
  • The stress test completes 640,000 critical-section operations while exercising frequent cross-worker migration.

• Breaking backward compatibility:

  • No. This change adds new APIs and only replaces an internal lock used by bvar's VarMap.

Test results

• bthread_mutex_unittest: 3 recursive mutex tests passed.
• brpc_bvar_mutex_unittest: 3 bvar regression tests passed.
• test_bvar: 64/64 tests passed.
• All related targets compiled successfully with GCC 10.

---

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 bvar worker starvation/deadlock risk by replacing the VarMap’s recursive pthread_mutex_t with a bthread-aware recursive mutex that tracks ownership by bthread ID (while still supporting native pthread callers). It also adds regression/stress tests and updates the test build rules across Make/CMake/Bazel to support the new coverage and avoid dependency cycles.

Changes:

• Introduce bthread_recursive_mutex_t plus bthread::RecursiveMutex, and implement recursive locking semantics on top of bthread_mutex_t.
• Switch bvar VarMap locking in src/bvar/variable.cpp to bthread::RecursiveMutex to prevent worker pthread blocking when metric callbacks yield.
• Add/adjust unit tests and build rules (Make/CMake/Bazel) including stubs to keep test_bvar runnable without the full bthread runtime.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/Makefile	Links test_bvar with stubbed recursive-mutex symbols; adds a custom link rule for brpc_bvar_mutex_unittest to avoid gtest_main.
test/CMakeLists.txt	Adds bvar_bthread_stubs.cpp to test_bvar; switches brpc_bvar_mutex_unittest to link gtest (not gtest_main).
test/bvar_bthread_stubs.cpp	Provides non-bthread-runtime implementations of bthread_recursive_mutex_* for test_bvar.
test/BUILD.bazel	Adds stub source to bvar unit tests; adds a dedicated Bazel test target for brpc_bvar_mutex_unittest; excludes it from the generated brpc unittest set.
test/bthread_mutex_unittest.cpp	Adds recursive-mutex unit/stress coverage, including yield/migration scenarios.
test/brpc_bvar_mutex_unittest.cpp	Adds regression tests for the bvar VarMap starvation scenario and recursive describe behavior (via fork/exec child modes).
src/bvar/variable.cpp	Replaces VarMap’s recursive pthread mutex with bthread::RecursiveMutex; updates locking sites.
src/bthread/types.h	Adds the bthread_recursive_mutex_t type.
src/bthread/recursive_mutex.h	Adds the public C API declarations and the bthread::RecursiveMutex C++ wrapper.
src/bthread/mutex.h	Includes the new recursive-mutex header (to expose the new API alongside existing mutex APIs).
src/bthread/mutex.cpp	Implements bthread_recursive_mutex_* atop bthread_mutex_t, tracking ownership by bthread ID or native pthread identity.
BUILD.bazel	Introduces :bthread_recursive_mutex header-only target; wires :bvar and :bthread deps to avoid a Bazel dependency cycle.

Comments suppressed due to low confidence (1)

test/brpc_bvar_mutex_unittest.cpp:305

• execl("/proc/self/exe", ...) is not portable and will cause this test to fail on platforms without /proc. Re-exec using the absolute path returned by butil::GetProcessAbsolutePath().

    if (pid == 0) {
        execl("/proc/self/exe", "brpc_bvar_mutex_unittest", child_argument,
              nullptr);
        _exit(127);
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[walterzhaoJR]

OK,this is a valid concern. The current //:bthread_recursive_mutex target exports only the headers, while the bthread_recursive_mutex_* implementations remain in //:bthread. As a result, a target depending only on //:bvar can compile but fail to link. The current bvar tests do not expose this because they explicitly link bvar_bthread_stubs.cpp.
Moving only the recursive-mutex functions into a separate library would not fully solve the cycle: they still depend on bthread_self() and bthread_mutex_*(), while //:bthread already depends on //:bvar.
A clean solution would require either:
splitting a bvar-independent mutex/runtime core out of //:bthread; or
combining the Bazel bvar and bthread targets, at the cost of losing standalone //:bvar.
Is standalone //:bvar considered a compatibility requirement? I would appreciate guidance on the preferred dependency boundary before restructuring this part.

[walterzhaoJR]

@chenBright

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (6)

test/bthread_mutex_unittest.cpp:221

• This increment is performed by many concurrent bthreads but currently updates a shared non-atomic counter, which is undefined behavior. Use an atomic increment so the assertion at the end of the test is meaningful.

        ++*args->counter;

test/bthread_mutex_unittest.cpp:245

• The shared counter should be atomic to match the concurrent increments from all stress bthreads, and to avoid undefined behavior.

    std::atomic<int> failures(0);
    std::atomic<int> migrations(0);
    int64_t counter = 0;
    RecursiveMutexStressArgs args = {
        &mutex, &ready, &start, &failures, &counter, &migrations};

test/bthread_mutex_unittest.cpp:260

• After making counter atomic, this assertion should load it atomically (otherwise it won't compile).

    EXPECT_EQ(static_cast<int64_t>(kThreadCount) * kIterations, counter);

BUILD.bazel:377

• //:bvar now instantiates bthread::RecursiveMutex (via src/bvar/variable.cpp), but bthread_recursive_mutex is a header-only Bazel target (srcs is empty) while the required bthread_recursive_mutex_* symbols are implemented in //:bthread (src/bthread/mutex.cpp). This means a Bazel target that previously linked only //:bvar can now compile but fail to link unless it also links //:bthread///:brpc, which is a backward-compatibility break and contradicts the PR description's "Breaking backward compatibility: No". Please either (a) make bthread_recursive_mutex provide the implementation (and keep it bvar-independent), or (b) add an explicit, non-cyclic link-time dependency guarantee for //:bvar users (e.g., via a split core mutex library or a documented/validated requirement).

cc_library(
    # Header-only interface so :bvar can use RecursiveMutex without depending on
    # :bthread (which itself depends on :bvar). Implementation lives in :bthread.
    name = "bthread_recursive_mutex",
    hdrs = [

test/brpc_bvar_mutex_unittest.cpp:372

• Using _exit in the normal (non-child) test runner path bypasses stdio flushing and C++/gtest cleanup, which can hide test output and make failures harder to diagnose. _exit is appropriate for the fork/exec child modes above, but the main test run should return normally.

    _exit(rc);

test/bthread_mutex_unittest.cpp:195

• counter is incremented concurrently by many bthreads in stress_recursive_mutex, but it's a plain int64_t* here. This is a data race/UB and can make the stress test flaky or appear to pass/fail nondeterministically. Use an atomic counter (or keep the counter update inside the mutex critical section with a single thread) so the test is well-defined.

This issue also appears in the following locations of the same file:

• line 221
• line 241
• line 260

    std::atomic<int>* failures;
    int64_t* counter;
    std::atomic<int>* migrations;