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

src/bvar/detail/exposed_ref.h

@@ -0,0 +1,101 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#ifndef BVAR_DETAIL_EXPOSED_REF_H_
+#define BVAR_DETAIL_EXPOSED_REF_H_
+
+#include <memory>
+#include "butil/macros.h"
+#include "butil/scoped_lock.h"
+#include "butil/synchronization/condition_variable.h"
+
+namespace bvar {
+namespace detail {
+
+// Indirection layer shared by Variable and MVariableBase that lets concurrent
+// readers (describe_exposed() / dump_exposed()) access an exposed object outside
+// the global map lock. That lock is a pthread mutex and must not wrap user
+// callbacks which may yield the bthread, otherwise it deadlocks (see
+// https://github.com/apache/brpc/issues/2888 for details).
+//
+// Protocol:
+//   - A reader increments the reference via acquire() while still holding the
+//     global map lock (so it is serialized with the owner's erase from the
+//     map), then releases the map lock, uses the returned pointer outside the
+//     lock, and finally calls release().
+//   - The owner's hide() erases itself from the map and then calls hide_and_wait(),
+//     which blocks until all references acquired before this point are released,
+//     guaranteeing the owner stays alive throughout the reader's use. The handle
+//     is single-use: once hidden it stays hidden, and the owner creates a fresh
+//     one on re-expose.
+template <typename T>
+class ExposedRef {
+public:
+    explicit ExposedRef(T* obj)
+        : _cond(&_mutex), _obj(obj), _nref(0), _hidden(false) {}
+
+    DISALLOW_COPY_AND_ASSIGN(ExposedRef);
+
+    // Must be called while holding the global map lock. Returns nullptr if the
+    // owner is being hidden/destructed. On a non-nullptr return the caller must
+    // call release() once it finishes using the pointer.
+    T* acquire() {
+        BAIDU_SCOPED_LOCK(_mutex);
+        if (_hidden) {
+            return nullptr;
+        }
+        ++_nref;
+        return _obj;
+    }
+
+    void release() {
+        BAIDU_SCOPED_LOCK(_mutex);
+        if (--_nref == 0 && _hidden) {
+            _cond.Broadcast();
+        }
+    }
+
+    // Called by the owner's hide() after erasing itself from the map. Blocks
+    // until all references acquired before this point are released.
+    void hide_and_wait() {
+        BAIDU_SCOPED_LOCK(_mutex);
+        _hidden = true;
+        while (_nref > 0) {
+            _cond.Wait();
+        }
+    }
+
+private:
+    butil::Mutex _mutex;
+    butil::ConditionVariable _cond;
+    T* _obj;
+    int _nref;
+    bool _hidden;
+};
+
+template <typename T>
+using SharedExposedRef = std::shared_ptr<ExposedRef<T>>;
+
+template <typename T>
+SharedExposedRef<T> make_exposed_ref(T* obj) {
+    return std::make_shared<ExposedRef<T>>(obj);
+}
+
+}  // namespace detail
+}  // namespace bvar
+
+#endif  // BVAR_DETAIL_EXPOSED_REF_H_

src/bvar/mvariable.cpp

@@ -69,11 +69,8 @@ DEFINE_uint32(max_multi_dimension_stats_count, 20000, "Max stats count of a mult
 BUTIL_VALIDATE_GFLAG(max_multi_dimension_stats_count,
                      validator_max_multi_dimension_stats_count);
 
-class MVarEntry {
-public:
-    MVarEntry() : var(nullptr) {}
-
-    MVariableBase* var;
+struct MVarEntry {
+    MVariableBase::SharedExposedRef ref;
 };
 
 typedef butil::FlatMap<std::string, MVarEntry> MVarMap;
@@ -119,12 +116,24 @@ std::string MVariableBase::get_description() {
 int MVariableBase::describe_exposed(const std::string& name,
                                 std::ostream& os) {
     MVarMapWithLock& m = get_mvar_map();
-    BAIDU_SCOPED_LOCK(m.mutex);
-    MVarEntry* entry = m.seek(name);
-    if (entry == nullptr) {
+    MVariableBase* var = nullptr;
+    SharedExposedRef ref;
+    {
+        BAIDU_SCOPED_LOCK(m.mutex);
+        MVarEntry* entry = m.seek(name);
+        if (entry == nullptr) {
+            return -1;
+        }
+        ref = entry->ref;
+        var = ref->acquire();
+    }
+    if (var == nullptr) {
         return -1;
     }
-    entry->var->describe(os);
+    // Call describe() outside the MVarMap lock to avoid deadlock when the user
+    // callback (e.g. Dumper) yields the bthread.
+    var->describe(os);
+    ref->release();
     return 0;
 }
 
@@ -149,8 +158,12 @@ int MVariableBase::expose_impl(const butil::StringPiece& prefix,
     // expose a variable more than once and calls to expose() are unlikely
     // to contend heavily.
 
-    // remove previous pointer from the map if needed.
+    // Remove previous exposure if needed (hide() waits for in-flight readers
+    // and invalidates `_ref`).
+    // Always start the new exposure with a fresh `_ref`,  because a previous
+    // hide() may have permanently hidden the old `_ref`.
     hide();
+    _ref = detail::make_exposed_ref(this);
 
     // Build the name.
     _name.clear();
@@ -164,7 +177,8 @@ int MVariableBase::expose_impl(const butil::StringPiece& prefix,
     to_underscored_name(&_name, name);
 
     if (count_exposed() > (size_t)FLAGS_bvar_max_multi_dimension_metric_number) {
-        LOG(ERROR) << "Too many metric seen, overflow detected, max metric count:" << FLAGS_bvar_max_multi_dimension_metric_number;
+        LOG(ERROR) << "Too many metric seen, overflow detected, max metric count:"
+                   << FLAGS_bvar_max_multi_dimension_metric_number;
         return -1;
     }
 
@@ -174,7 +188,7 @@ int MVariableBase::expose_impl(const butil::StringPiece& prefix,
         MVarEntry* entry = m.seek(_name);
         if (entry == nullptr) {
             entry = &m[_name];
-            entry->var = this;
+            entry->ref = _ref;
             return 0;
         }
     }
@@ -200,14 +214,23 @@ bool MVariableBase::hide() {
     }
 
     MVarMapWithLock& m = get_mvar_map();
-    BAIDU_SCOPED_LOCK(m.mutex);
-    MVarEntry* entry = m.seek(_name);
-    if (entry) {
-        CHECK_EQ(1UL, m.erase(_name));
-    } else {
-        CHECK(false) << "`" << _name << "' must exist";
+    {
+        BAIDU_SCOPED_LOCK(m.mutex);
+        MVarEntry* entry = m.seek(_name);
+        if (entry) {
+            CHECK_EQ(1UL, m.erase(_name));
+        } else {
+            CHECK(false) << "`" << _name << "' must exist";
+        }
     }
     _name.clear();
+    // Remove previous exposure if needed (hide() waits for in-flight readers
+    // and invalidates `_ref`).
+    // Always start the new exposure with a fresh `_ref`,  because a previous
+    // hide() may have permanently hidden the old `_ref`.
+    if (_ref != nullptr) {
+        _ref->hide_and_wait();
+    }
     return true;
 }
 
@@ -253,11 +276,22 @@ size_t MVariableBase::dump_exposed(Dumper* dumper, const DumpOptions* options) {
     list_exposed(&mvars);
     size_t n = 0;
     for (auto& mvar : mvars) {
-        MVarMapWithLock& m = get_mvar_map();
-        BAIDU_SCOPED_LOCK(m.mutex);
-        MVarEntry* entry = m.seek(mvar);
-        if (entry) {
-            n += entry->var->dump(dumper, &opt);
+        MVariableBase* var = nullptr;
+        SharedExposedRef ref;
+        {
+            MVarMapWithLock& m = get_mvar_map();
+            BAIDU_SCOPED_LOCK(m.mutex);
+            MVarEntry* entry = m.seek(mvar);
+            if (entry) {
+                ref = entry->ref;
+                var = ref->acquire();
+            }
+        }
+        if (var != nullptr) {
+            // Call dump() outside the MVarMap lock to avoid deadlock when the dump()
+            // yields the bthread.
+            n += var->dump(dumper, &opt);
+            ref->release();
         }
         if (n > static_cast<size_t>(FLAGS_bvar_max_dump_multi_dimension_metric_number)) {
             LOG(WARNING) << "truncated because of exceed max dump multi dimension label number["

src/bvar/mvariable.h

@@ -24,8 +24,11 @@
 #include <sstream>                      // std::ostringstream
 #include <list>                         // std::list
 #include <string>                       // std::string
+#include <vector>                       // std::vector
+#include <memory>                       // std::shared_ptr
 #include "butil/macros.h"               // DISALLOW_COPY_AND_ASSIGN
 #include "butil/strings/string_piece.h" // butil::StringPiece
+#include "bvar/detail/exposed_ref.h"     // detail::ExposedRef
 
 namespace bvar {
 
@@ -34,8 +37,16 @@ struct DumpOptions;
 
 class MVariableBase {
 public:
+    // Shared, single-use handle that lets describe_exposed()/dump_exposed()
+    // call describe()/dump() OUTSIDE the global MVarMap lock (issue #2888).
+    using SharedExposedRef = detail::SharedExposedRef<MVariableBase>;
+
     MVariableBase() = default;
 
+    // mbvar uses bvar, bvar uses TLS, thus copying/assignment need to copy TLS stuff as well,
+    // which is heavy. We disable copying/assignment now.
+    DISALLOW_COPY_AND_ASSIGN(MVariableBase);
+
     virtual ~MVariableBase();
 
     // Implement this method to print the mvariable info into ostream.
@@ -107,10 +118,8 @@ class MVariableBase {
 
 protected:
     std::string _name;
-
-    // mbvar uses bvar, bvar uses TLS, thus copying/assignment need to copy TLS stuff as well,
-    // which is heavy. We disable copying/assignment now. 
-    DISALLOW_COPY_AND_ASSIGN(MVariableBase);
+    // Shared indirection handle for describe()/dump() outside the MVarMap lock.
+    SharedExposedRef _ref;
 };
 
 template <typename KeyType>