From 19e934bdebbbf84fe74222ab5dbc34c7ec49d051 Mon Sep 17 00:00:00 2001 From: chenBright Date: Sat, 22 Aug 2026 22:52:26 +0800 Subject: [PATCH] Fix potential bvar deadlock by running describe()/dump() outside the global VarMap lock --- src/bvar/detail/exposed_ref.h | 101 ++++++++++++++++++++++++++++++ src/bvar/mvariable.cpp | 80 +++++++++++++++++------- src/bvar/mvariable.h | 17 +++-- src/bvar/variable.cpp | 107 +++++++++++++++++++++++--------- src/bvar/variable.h | 17 +++-- test/bthread_unittest.cpp | 40 ++++++++++++ test/bvar_variable_unittest.cpp | 54 ++++++++++++++++ 7 files changed, 353 insertions(+), 63 deletions(-) create mode 100644 src/bvar/detail/exposed_ref.h diff --git a/src/bvar/detail/exposed_ref.h b/src/bvar/detail/exposed_ref.h new file mode 100644 index 0000000000..a6a036b65b --- /dev/null +++ b/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 +#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 +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 +using SharedExposedRef = std::shared_ptr>; + +template +SharedExposedRef make_exposed_ref(T* obj) { + return std::make_shared>(obj); +} + +} // namespace detail +} // namespace bvar + +#endif // BVAR_DETAIL_EXPOSED_REF_H_ diff --git a/src/bvar/mvariable.cpp b/src/bvar/mvariable.cpp index 45517a28e8..6fbceb077f 100644 --- a/src/bvar/mvariable.cpp +++ b/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 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(FLAGS_bvar_max_dump_multi_dimension_metric_number)) { LOG(WARNING) << "truncated because of exceed max dump multi dimension label number[" diff --git a/src/bvar/mvariable.h b/src/bvar/mvariable.h index ec6eb1cec0..95c79264b6 100644 --- a/src/bvar/mvariable.h +++ b/src/bvar/mvariable.h @@ -24,8 +24,11 @@ #include // std::ostringstream #include // std::list #include // std::string +#include // std::vector +#include // 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() = 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 diff --git a/src/bvar/variable.cpp b/src/bvar/variable.cpp index 17ffed302b..688997d62d 100644 --- a/src/bvar/variable.cpp +++ b/src/bvar/variable.cpp @@ -64,9 +64,11 @@ BAIDU_CASSERT(!(SUB_MAP_COUNT & (SUB_MAP_COUNT - 1)), must_be_power_of_2); class VarEntry { public: - VarEntry() : var(nullptr), display_filter(DISPLAY_ON_ALL) {} + VarEntry() : display_filter(DISPLAY_ON_ALL) {} - Variable* var; + // Indirection handle shared with the Variable. describe_exposed() acquires + // it under the VarMap lock and calls describe() outside the lock. + Variable::SharedExposedRef ref; DisplayFilter display_filter; }; @@ -79,12 +81,7 @@ struct VarMapWithLock : public VarMap { if (init(1024) != 0) { LOG(WARNING) << "Fail to init VarMap"; } - - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - pthread_mutex_init(&mutex, &attr); - pthread_mutexattr_destroy(&attr); + pthread_mutex_init(&mutex, nullptr); } }; @@ -104,7 +101,8 @@ inline size_t sub_map_index(const std::string& str) { return 0; } size_t h = 0; - // we're assume that str is ended with '\0', which may not be in general + // We assume that str is ended with '\0', which may not be in general; + // otherwise the hash stops early. for (const char* p = str.c_str(); *p; ++p) { h = h * 5 + *p; } @@ -140,8 +138,12 @@ int Variable::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(); @@ -160,7 +162,7 @@ int Variable::expose_impl(const butil::StringPiece& prefix, VarEntry* entry = m.seek(_name); if (entry == nullptr) { entry = &m[_name]; - entry->var = this; + entry->ref = _ref; entry->display_filter = display_filter; return 0; } @@ -189,14 +191,23 @@ bool Variable::hide() { return false; } VarMapWithLock& m = get_var_map(_name); - BAIDU_SCOPED_LOCK(m.mutex); - VarEntry* entry = m.seek(_name); - if (entry) { - CHECK_EQ(1UL, m.erase(_name)); - } else { - CHECK(false) << "`" << _name << "' must exist"; + { + BAIDU_SCOPED_LOCK(m.mutex); + VarEntry* 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; } @@ -249,15 +260,28 @@ int Variable::describe_exposed(const std::string& name, std::ostream& os, bool quote_string, DisplayFilter display_filter) { VarMapWithLock& m = get_var_map(name); - BAIDU_SCOPED_LOCK(m.mutex); - VarEntry* p = m.seek(name); - if (p == nullptr) { - return -1; + Variable* var = nullptr; + SharedExposedRef ref; + { + BAIDU_SCOPED_LOCK(m.mutex); + VarEntry* p = m.seek(name); + if (p == nullptr) { + return -1; + } + if (!(display_filter & p->display_filter)) { + return -1; + } + ref = p->ref; + var = ref->acquire(); } - if (!(display_filter & p->display_filter)) { + if (var == nullptr) { + // The variable is being destructed. return -1; } - p->var->describe(os, quote_string); + // Call describe() outside the VarMap lock to avoid deadlock when the user + // callback (e.g. PassiveStatus) yields the bthread. + var->describe(os, quote_string); + ref->release(); return 0; } @@ -289,23 +313,44 @@ int Variable::describe_series_exposed(const std::string& name, std::ostream& os, const SeriesOptions& options) { VarMapWithLock& m = get_var_map(name); - BAIDU_SCOPED_LOCK(m.mutex); - VarEntry* p = m.seek(name); - if (p == nullptr) { + Variable* var = nullptr; + SharedExposedRef ref; + { + BAIDU_SCOPED_LOCK(m.mutex); + VarEntry* p = m.seek(name); + if (p == nullptr) { + return -1; + } + ref = p->ref; + var = ref->acquire(); + } + if (var == nullptr) { return -1; } - return p->var->describe_series(os, options); + const int rc = var->describe_series(os, options); + ref->release(); + return rc; } #ifdef BAIDU_INTERNAL int Variable::get_exposed(const std::string& name, boost::any* value) { VarMapWithLock& m = get_var_map(name); - BAIDU_SCOPED_LOCK(m.mutex); - VarEntry* p = m.seek(name); - if (p == nullptr) { + Variable* var = nullptr; + SharedExposedRef ref; + { + BAIDU_SCOPED_LOCK(m.mutex); + VarEntry* p = m.seek(name); + if (p == nullptr) { + return -1; + } + ref = p->ref; + var = ref->acquire(); + } + if (var == nullptr) { return -1; } - p->var->get_value(value); + var->get_value(value); + ref->release(); return 0; } #endif diff --git a/src/bvar/variable.h b/src/bvar/variable.h index b13f000f99..cbc14cfbb1 100644 --- a/src/bvar/variable.h +++ b/src/bvar/variable.h @@ -23,9 +23,11 @@ #include // std::ostream #include // std::string #include // std::vector +#include // std::shared_ptr #include #include "butil/macros.h" // DISALLOW_COPY_AND_ASSIGN #include "butil/strings/string_piece.h" // butil::StringPiece +#include "bvar/detail/exposed_ref.h" // detail::ExposedRef #ifdef BAIDU_INTERNAL #include @@ -117,7 +119,14 @@ struct SeriesOptions { // safely (provided that there's no non-const methods going on). class Variable { public: - Variable() {} + using SharedExposedRef = detail::SharedExposedRef; + + Variable() = default; + + // 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(Variable); + virtual ~Variable(); // Implement this method to print the variable into ostream. @@ -234,10 +243,8 @@ class Variable { private: std::string _name; - - // 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(Variable); + // Shared indirection handle for calling describe() outside the VarMap lock. + SharedExposedRef _ref; }; // Make name only use lowercased alphabets / digits / underscores, and append diff --git a/test/bthread_unittest.cpp b/test/bthread_unittest.cpp index b6d5ca7f6e..f2b92ef0bd 100644 --- a/test/bthread_unittest.cpp +++ b/test/bthread_unittest.cpp @@ -22,9 +22,12 @@ #include "butil/macros.h" #include "butil/logging.h" #include "gperftools_helper.h" +#include +#include #include "bthread/bthread.h" #include "bthread/unstable.h" #include "bthread/task_meta.h" +#include "bvar/bvar.h" int main(int argc, char* argv[]) { testing::InitGoogleTest(&argc, argv); @@ -701,4 +704,41 @@ TEST_F(BthreadTest, trace) { } #endif // BRPC_BTHREAD_TRACER +// Regression test for https://github.com/apache/brpc/issues/2888 . +// Reproduce the real deadlock: many bthreads (>> bthread_concurrency) call +// describe_exposed() on the same variable concurrently, and the user callback +// yields the bthread. In the buggy version describe() runs while holding the +// global VarMap pthread mutex, so once a bthread yields inside the callback the +// pthread worker picks up another describing bthread that blocks on the same +// submap mutex; with enough bthreads all workers get stuck and the process +// deadlocks. With the fix (describe() runs OUTSIDE the lock) this finishes. +int yielding_getfn(void*) { + bthread_usleep(500); + return 0; +} + +void* describe_same_var(void*) { + std::ostringstream os; + bvar::Variable::describe_exposed("bthread_describe_deadlock", os); + return nullptr; +} + +TEST_F(BthreadTest, describe_exposed_yields_in_bthread_no_deadlock) { + bvar::PassiveStatus ps( + "bthread_describe_deadlock", yielding_getfn, nullptr); + + // n >> bthread_concurrency, so that in the buggy version every worker ends + // up blocked on the same submap mutex held by a yielded bthread. + const int N = 1000; + std::vector tids(N); + for (int i = 0; i < N; ++i) { + ASSERT_EQ(0, bthread_start_background( + &tids[i], nullptr, describe_same_var, nullptr)); + } + for (int i = 0; i < N; ++i) { + ASSERT_EQ(0, bthread_join(tids[i], nullptr)); + } + // Reaching here (instead of hanging) means there was no deadlock. +} + } // namespace diff --git a/test/bvar_variable_unittest.cpp b/test/bvar_variable_unittest.cpp index 55a0e4b6bd..20226854b6 100644 --- a/test/bvar_variable_unittest.cpp +++ b/test/bvar_variable_unittest.cpp @@ -18,9 +18,11 @@ // Date: Fri Jul 24 17:19:40 CST 2015 #include // pthread_* +#include // usleep #include #include +#include #include #include #include "butil/time.h" @@ -408,6 +410,58 @@ TEST_F(VariableTest, recursive_mutex) { LOG(INFO) << "Each recursive mutex lock/unlock pair take " << timer.n_elapsed() / N << "ns"; } + +struct BlockingDescribeCtx { + butil::atomic entered; + butil::atomic release; + BlockingDescribeCtx() : entered(false), release(false) {} +}; + +int blocking_getfn(void* arg) { + BlockingDescribeCtx* c = static_cast(arg); + c->entered.store(true); + while (!c->release.load()) { + usleep(1000); + } + return 42; +} + +// A Variable dtor must block until all in-flight describe_exposed() finish, +// otherwise a concurrent describe() would use-after-free the Variable. +TEST_F(VariableTest, dtor_waits_for_inflight_describe) { + BlockingDescribeCtx ctx; + auto a = new bvar::PassiveStatus( + "lockfree_describe_b", blocking_getfn, &ctx); + + std::thread describer([]{ + std::ostringstream os; + bvar::Variable::describe_exposed("lockfree_describe_b", os); + }); + while (!ctx.entered.load()) { + usleep(1000); + } + + // Destruct in another thread. It must block inside hide()'s hide_and_wait() + // until the in-flight describe finishes. + butil::atomic destructed(false); + std::thread destroyer([&]{ + delete a; + destructed.store(true); + }); + + // While the callback is still blocked, the dtor must not have completed. + usleep(200 * 1000); + ASSERT_FALSE(destructed.load()) + << "Variable dtor did not wait for the in-flight describe() to finish."; + + // Release the callback; describe() returns, ref-count drops to 0, and the + // dtor is unblocked. + ctx.release.store(true); + describer.join(); + destroyer.join(); + + ASSERT_TRUE(destructed.load()); +} } // namespace int main(int argc, char** argv) {