This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 03449c72 Add ScopedExposedRef and synchronize bvar counting (#3710)
03449c72 is described below
commit 03449c72f1eb83cd5becf2defd6b1882eaec0c06
Author: Bright Chen <[email protected]>
AuthorDate: Fri Nov 6 14:19:40 2026 +0800
Add ScopedExposedRef and synchronize bvar counting (#3710)
* Add ScopedExposedRef and synchronize bvar counting
* Add <memory>
---
src/bvar/detail/exposed_ref.h | 56 +++++++++-
src/bvar/mvariable.cpp | 26 ++---
src/bvar/mvariable.h | 1 +
src/bvar/variable.cpp | 35 ++----
src/bvar/variable.h | 1 +
test/bvar_exposed_ref_unittest.cpp | 220 +++++++++++++++++++++++++++++++++++++
6 files changed, 298 insertions(+), 41 deletions(-)
diff --git a/src/bvar/detail/exposed_ref.h b/src/bvar/detail/exposed_ref.h
index a6a036b6..f964da81 100644
--- a/src/bvar/detail/exposed_ref.h
+++ b/src/bvar/detail/exposed_ref.h
@@ -18,7 +18,9 @@
#ifndef BVAR_DETAIL_EXPOSED_REF_H_
#define BVAR_DETAIL_EXPOSED_REF_H_
+#include <cstddef>
#include <memory>
+#include <utility>
#include "butil/macros.h"
#include "butil/scoped_lock.h"
#include "butil/synchronization/condition_variable.h"
@@ -36,7 +38,8 @@ namespace detail {
// - 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().
+// lock, and finally calls release(). ScopedExposedRef manages this pairing
+// automatically, including on early returns and exceptions.
// - 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
@@ -90,6 +93,57 @@ private:
template <typename T>
using SharedExposedRef = std::shared_ptr<ExposedRef<T>>;
+// Owns one acquired reader reference and keeps its ExposedRef alive. Construct
+// or reset with a non-empty handle while holding the global map lock, then use
+// the object outside that lock. Destruction/reset releases only a successfully
+// acquired reference. Moving transfers ownership without acquiring again.
+template <typename T>
+class ScopedExposedRef {
+public:
+ explicit ScopedExposedRef(const SharedExposedRef<T>& ref = nullptr)
+ : _ref(ref), _obj(_ref != nullptr ? _ref->acquire() : nullptr) {}
+
+ ~ScopedExposedRef() {
+ if (_obj != nullptr) {
+ _ref->release();
+ }
+ }
+
+ DISALLOW_COPY_AND_ASSIGN(ScopedExposedRef);
+
+ ScopedExposedRef(ScopedExposedRef&& other) noexcept
+ : _ref(std::move(other._ref)), _obj(other._obj) {
+ other._obj = nullptr;
+ }
+
+ ScopedExposedRef& operator=(ScopedExposedRef&& other) noexcept {
+ ScopedExposedRef tmp(std::move(other));
+ swap(tmp);
+ return *this;
+ }
+
+ void reset(const SharedExposedRef<T>& ref = nullptr) {
+ // Acquire before releasing the previous reference, even if both
+ // handles are the same.
+ ScopedExposedRef tmp(ref);
+ swap(tmp);
+ }
+
+ T* get() const { return _obj; }
+ T* operator->() const { return _obj; }
+ bool operator==(std::nullptr_t) const { return _obj == nullptr; }
+ bool operator!=(std::nullptr_t) const { return _obj != nullptr; }
+
+private:
+ void swap(ScopedExposedRef& other) noexcept {
+ _ref.swap(other._ref);
+ std::swap(_obj, other._obj);
+ }
+
+ SharedExposedRef<T> _ref;
+ T* _obj;
+};
+
template <typename T>
SharedExposedRef<T> make_exposed_ref(T* obj) {
return std::make_shared<ExposedRef<T>>(obj);
diff --git a/src/bvar/mvariable.cpp b/src/bvar/mvariable.cpp
index 39bab85e..7c935cd3 100644
--- a/src/bvar/mvariable.cpp
+++ b/src/bvar/mvariable.cpp
@@ -118,16 +118,14 @@ std::string MVariableBase::get_description() {
int MVariableBase::describe_exposed(const std::string& name,
std::ostream& os) {
MVarMapWithLock& m = get_mvar_map();
- MVariableBase* var = nullptr;
- SharedExposedRef ref;
+ ScopedExposedRef var;
{
BAIDU_SCOPED_LOCK(m.mutex);
MVarEntry* entry = m.seek(name);
if (entry == nullptr) {
return -1;
}
- ref = entry->ref;
- var = ref->acquire();
+ var.reset(entry->ref);
}
if (var == nullptr) {
return -1;
@@ -135,7 +133,6 @@ int MVariableBase::describe_exposed(const std::string& name,
// 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;
}
@@ -246,7 +243,7 @@ bool MVariableBase::hide() {
#ifdef UNIT_TEST
void MVariableBase::hide_all() {
struct MVariableToHide {
- MVariableBase* variable;
+ ScopedExposedRef variable;
SharedExposedRef ref;
};
@@ -260,7 +257,7 @@ void MVariableBase::hide_all() {
for (MVarMap::const_iterator it = m.begin(); it != m.end(); ++it) {
MVariableToHide variable;
variable.ref = it->second.ref;
- variable.variable = variable.ref->acquire();
+ variable.variable.reset(variable.ref);
if (variable.variable != nullptr) {
variables.push_back(std::move(variable));
}
@@ -269,12 +266,12 @@ void MVariableBase::hide_all() {
}
for (auto& variable : variables) {
- // Touch the owner while the reference taken by acquire() above is
still
- // held: release() lets a concurrent destructor run to completion, and
+ // Touch the owner while the scoped reference is still held: resetting
+ // it lets a concurrent destructor run to completion, and
// hide_and_wait() blocks until the last reference is gone, so it has
to
- // come after release() rather than before it.
+ // come after reset() rather than before it.
variable.variable->_name.clear();
- variable.ref->release();
+ variable.variable.reset();
variable.ref->hide_and_wait();
}
}
@@ -364,22 +361,19 @@ int MVariableBase::dump_exposed(Dumper* dumper, const
DumpOptions* options) {
dumper,
static_cast<size_t>(FLAGS_bvar_max_dump_multi_dimension_metric_number));
size_t n = 0;
for (size_t i = 0; i < mvars.size(); ++i) {
- MVariableBase* var = nullptr;
- SharedExposedRef ref;
+ ScopedExposedRef var;
{
MVarMapWithLock& m = get_mvar_map();
BAIDU_SCOPED_LOCK(m.mutex);
MVarEntry* entry = m.seek(mvars[i]);
if (entry) {
- ref = entry->ref;
- var = ref->acquire();
+ var.reset(entry->ref);
}
}
if (var != nullptr) {
// Call dump() outside the MVarMap lock to avoid deadlock when the
dump()
// yields the bthread.
n += var->dump(&limited_dumper, &opt);
- ref->release();
}
if (limited_dumper.exhausted()) {
// An exact fit on the last mvar has lost nothing.
diff --git a/src/bvar/mvariable.h b/src/bvar/mvariable.h
index 47f8474a..370078fe 100644
--- a/src/bvar/mvariable.h
+++ b/src/bvar/mvariable.h
@@ -40,6 +40,7 @@ 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>;
+ using ScopedExposedRef = detail::ScopedExposedRef<MVariableBase>;
MVariableBase() = default;
diff --git a/src/bvar/variable.cpp b/src/bvar/variable.cpp
index 29e6ad9a..be1bb6e2 100644
--- a/src/bvar/variable.cpp
+++ b/src/bvar/variable.cpp
@@ -304,6 +304,7 @@ size_t Variable::count_exposed() {
size_t n = 0;
VarMapWithLock* var_maps = get_var_maps();
for (size_t i = 0; i < SUB_MAP_COUNT; ++i) {
+ BAIDU_SCOPED_LOCK(var_maps[i].mutex);
n += var_maps[i].size();
}
return n;
@@ -313,8 +314,7 @@ int Variable::describe_exposed(const std::string& name,
std::ostream& os,
bool quote_string,
DisplayFilter display_filter) {
VarMapWithLock& m = get_var_map(name);
- Variable* var = nullptr;
- SharedExposedRef ref;
+ ScopedExposedRef var;
{
BAIDU_SCOPED_LOCK(m.mutex);
VarEntry* p = m.seek(name);
@@ -324,8 +324,7 @@ int Variable::describe_exposed(const std::string& name,
std::ostream& os,
if (!(display_filter & p->display_filter)) {
return -1;
}
- ref = p->ref;
- var = ref->acquire();
+ var.reset(p->ref);
}
if (var == nullptr) {
// The variable is being destructed.
@@ -334,7 +333,6 @@ int Variable::describe_exposed(const std::string& name,
std::ostream& os,
// 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;
}
@@ -366,44 +364,37 @@ int Variable::describe_series_exposed(const std::string&
name,
std::ostream& os,
const SeriesOptions& options) {
VarMapWithLock& m = get_var_map(name);
- Variable* var = nullptr;
- SharedExposedRef ref;
+ ScopedExposedRef var;
{
BAIDU_SCOPED_LOCK(m.mutex);
VarEntry* p = m.seek(name);
if (p == nullptr) {
return -1;
}
- ref = p->ref;
- var = ref->acquire();
+ var.reset(p->ref);
}
if (var == nullptr) {
return -1;
}
- const int rc = var->describe_series(os, options);
- ref->release();
- return rc;
+ return var->describe_series(os, options);
}
#ifdef BAIDU_INTERNAL
int Variable::get_exposed(const std::string& name, boost::any* value) {
VarMapWithLock& m = get_var_map(name);
- Variable* var = nullptr;
- SharedExposedRef ref;
+ ScopedExposedRef var;
{
BAIDU_SCOPED_LOCK(m.mutex);
VarEntry* p = m.seek(name);
if (p == nullptr) {
return -1;
}
- ref = p->ref;
- var = ref->acquire();
+ var.reset(p->ref);
}
if (var == nullptr) {
return -1;
}
var->get_value(value);
- ref->release();
return 0;
}
#endif
@@ -574,8 +565,7 @@ static int dump_exposed_one(Dumper* dumper,
const DumpOptions& options,
const std::string& name) {
VarMapWithLock& m = get_var_map(name);
- Variable* var = nullptr;
- Variable::SharedExposedRef ref;
+ Variable::ScopedExposedRef var;
{
BAIDU_SCOPED_LOCK(m.mutex);
VarEntry* p = m.seek(name);
@@ -585,8 +575,7 @@ static int dump_exposed_one(Dumper* dumper,
if (!(options.display_filter & p->display_filter)) {
return 0;
}
- ref = p->ref;
- var = ref->acquire();
+ var.reset(p->ref);
}
if (var == nullptr) {
// The variable is being destructed.
@@ -594,9 +583,7 @@ static int dump_exposed_one(Dumper* dumper,
}
// Dump outside the VarMap lock to avoid deadlock when the user callback
// (e.g. PassiveStatus) yields the bthread, same as describe_exposed().
- bool ok = var->dump(dumper, options, name);
- ref->release();
- return ok ? 1 : -1;
+ return var->dump(dumper, options, name) ? 1 : -1;
}
// Tees everything going through the wrapped dumper into `info`, implementing
diff --git a/src/bvar/variable.h b/src/bvar/variable.h
index 6afcf248..2f3f6680 100644
--- a/src/bvar/variable.h
+++ b/src/bvar/variable.h
@@ -212,6 +212,7 @@ struct SeriesOptions {
class Variable {
public:
using SharedExposedRef = detail::SharedExposedRef<Variable>;
+ using ScopedExposedRef = detail::ScopedExposedRef<Variable>;
Variable() = default;
diff --git a/test/bvar_exposed_ref_unittest.cpp
b/test/bvar_exposed_ref_unittest.cpp
new file mode 100644
index 00000000..69f194ea
--- /dev/null
+++ b/test/bvar_exposed_ref_unittest.cpp
@@ -0,0 +1,220 @@
+// 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.
+
+#include <memory>
+#include <sstream>
+#include <string>
+#include <type_traits>
+#include <utility>
+#include <vector>
+#include <gtest/gtest.h>
+#include "bvar/detail/exposed_ref.h"
+#include "bvar/mvariable.h"
+#include "bvar/variable.h"
+
+namespace {
+
+using bvar::detail::ScopedExposedRef;
+using bvar::detail::make_exposed_ref;
+
+static_assert(!std::is_copy_constructible<ScopedExposedRef<int>>::value,
+ "ScopedExposedRef must not duplicate ownership");
+static_assert(!std::is_copy_assignable<ScopedExposedRef<int>>::value,
+ "ScopedExposedRef must not duplicate ownership");
+
+TEST(ScopedExposedRefTest, scope_exit_and_early_return) {
+ std::string value = "value";
+ auto ref = make_exposed_ref(&value);
+ {
+ ScopedExposedRef<std::string> first(ref);
+ EXPECT_EQ(&value, first.get());
+ EXPECT_EQ(value.size(), first->size());
+ const auto read = [&]() {
+ ScopedExposedRef<std::string> second(ref);
+ EXPECT_EQ(2, ref->_nref);
+ return second->size();
+ };
+ EXPECT_EQ(value.size(), read());
+ EXPECT_EQ(1, ref->_nref);
+ }
+ EXPECT_EQ(0, ref->_nref);
+}
+
+TEST(ScopedExposedRefTest, keeps_handle_alive) {
+ int value = 42;
+ auto ref = make_exposed_ref(&value);
+ std::weak_ptr<bvar::detail::ExposedRef<int>> weak_ref = ref;
+ {
+ ScopedExposedRef<int> reader(ref);
+ ref.reset();
+ EXPECT_FALSE(weak_ref.expired());
+ EXPECT_EQ(&value, reader.get());
+ }
+ EXPECT_TRUE(weak_ref.expired());
+}
+
+TEST(ScopedExposedRefTest, reset_and_hidden_handle) {
+ int value = 42;
+ auto ref = make_exposed_ref(&value);
+ auto hidden_ref = make_exposed_ref(&value);
+ hidden_ref->hide_and_wait();
+ {
+ ScopedExposedRef<int> reader;
+ EXPECT_TRUE(reader == nullptr);
+ EXPECT_EQ(nullptr, reader.get());
+ reader.reset();
+ reader.reset(ref);
+ EXPECT_TRUE(reader != nullptr);
+ EXPECT_EQ(1, ref->_nref);
+ reader.reset(ref);
+ EXPECT_EQ(1, ref->_nref);
+ reader.reset(hidden_ref);
+ EXPECT_TRUE(reader == nullptr);
+ EXPECT_EQ(0, ref->_nref);
+ EXPECT_EQ(0, hidden_ref->_nref);
+ reader.reset();
+ reader.reset();
+ }
+ EXPECT_EQ(0, hidden_ref->_nref);
+}
+
+TEST(ScopedExposedRefTest, move_transfers_ownership) {
+ int first_value = 1;
+ int second_value = 2;
+ auto first_ref = make_exposed_ref(&first_value);
+ auto second_ref = make_exposed_ref(&second_value);
+ {
+ ScopedExposedRef<int> first(first_ref);
+ ScopedExposedRef<int> moved(std::move(first));
+ EXPECT_TRUE(first == nullptr);
+ EXPECT_EQ(&first_value, moved.get());
+ EXPECT_EQ(1, first_ref->_nref);
+
+ ScopedExposedRef<int> second(second_ref);
+ second = std::move(moved);
+ EXPECT_TRUE(moved == nullptr);
+ EXPECT_EQ(&first_value, second.get());
+ EXPECT_EQ(1, first_ref->_nref);
+ EXPECT_EQ(0, second_ref->_nref);
+
+ auto* self = &second;
+ second = std::move(*self);
+ EXPECT_EQ(&first_value, second.get());
+ EXPECT_EQ(1, first_ref->_nref);
+
+ // hide_all() stores guards in a vector, which may move them again.
+ std::vector<ScopedExposedRef<int>> readers;
+ readers.push_back(std::move(second));
+ readers.emplace_back(first_ref);
+ EXPECT_TRUE(second == nullptr);
+ EXPECT_EQ(2, first_ref->_nref);
+ }
+ EXPECT_EQ(0, first_ref->_nref);
+ EXPECT_EQ(0, second_ref->_nref);
+}
+
+#if GTEST_HAS_EXCEPTIONS
+
+struct CallbackError {};
+
+// There are no concurrent readers in these tests. Drain leaked references on
+// failure so a regression reports an assertion instead of hanging in hide().
+template <typename T>
+void expect_no_readers(const bvar::detail::SharedExposedRef<T>& ref) {
+ EXPECT_EQ(0, ref->_nref);
+ while (ref->_nref > 0) {
+ ref->release();
+ }
+}
+
+class ThrowingVariable : public bvar::Variable {
+public:
+ ~ThrowingVariable() override { hide(); }
+ void describe(std::ostream&, bool) const override {
+ throw CallbackError();
+ }
+ int describe_series(std::ostream&,
+ const bvar::SeriesOptions&) const override {
+ throw CallbackError();
+ }
+#ifdef BAIDU_INTERNAL
+ void get_value(boost::any*) const override { throw CallbackError(); }
+#endif
+};
+
+class ThrowingMVariable : public bvar::MVariableBase {
+public:
+ ~ThrowingMVariable() override { hide(); }
+ void describe(std::ostream&) override { throw CallbackError(); }
+ size_t dump(bvar::Dumper*, const bvar::DumpOptions*) override {
+ throw CallbackError();
+ }
+};
+
+class DiscardingDumper : public bvar::Dumper {
+public:
+ bool dump(const std::string&, const butil::StringPiece&) override {
+ return true;
+ }
+};
+
+TEST(ScopedExposedRefTest, variable_callbacks_release_on_exception) {
+ ThrowingVariable var;
+ ASSERT_EQ(0, var.expose("scoped_ref_throwing_variable"));
+ std::ostringstream os;
+ EXPECT_THROW(bvar::Variable::describe_exposed(var.name(), os),
+ CallbackError);
+ expect_no_readers(var._ref);
+
+ bvar::SeriesOptions series_options;
+ EXPECT_THROW(bvar::Variable::describe_series_exposed(
+ var.name(), os, series_options), CallbackError);
+ expect_no_readers(var._ref);
+
+ DiscardingDumper dumper;
+ bvar::DumpOptions dump_options;
+ dump_options.white_wildcards = var.name();
+ EXPECT_THROW(bvar::Variable::dump_exposed(&dumper, &dump_options),
+ CallbackError);
+ expect_no_readers(var._ref);
+#ifdef BAIDU_INTERNAL
+ boost::any value;
+ EXPECT_THROW(bvar::Variable::get_exposed(var.name(), &value),
+ CallbackError);
+ expect_no_readers(var._ref);
+#endif
+ EXPECT_TRUE(var.hide());
+}
+
+TEST(ScopedExposedRefTest, mvariable_callbacks_release_on_exception) {
+ ThrowingMVariable var;
+ ASSERT_EQ(0, var.expose("scoped_ref_throwing_mvariable"));
+ std::ostringstream os;
+ EXPECT_THROW(bvar::MVariableBase::describe_exposed(var.name(), os),
+ CallbackError);
+ expect_no_readers(var._ref);
+
+ DiscardingDumper dumper;
+ EXPECT_THROW(bvar::MVariableBase::dump_exposed(&dumper, nullptr),
+ CallbackError);
+ expect_no_readers(var._ref);
+ EXPECT_TRUE(var.hide());
+}
+
+#endif // GTEST_HAS_EXCEPTIONS
+
+} // namespace
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
