Copilot commented on code in PR #3710:
URL: https://github.com/apache/brpc/pull/3710#discussion_r4477926820
##########
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();
Review Comment:
`count_exposed()` is now synchronized with per-submap locks to address
concurrent modification. There’s no test in this PR that exercises
`count_exposed()` under concurrent expose/hide activity; adding a multithreaded
unit test (ideally runnable under TSAN) would help prevent regressions and
validate that the new locking removes the data race.
##########
src/bvar/detail/exposed_ref.h:
##########
@@ -90,6 +93,57 @@ class ExposedRef {
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; }
Review Comment:
`ScopedExposedRef` currently supports `== nullptr`/`!= nullptr` but not `if
(ref)` style checks. Adding an `explicit operator bool() const` would make the
type more pointer-like and easier to use consistently across the codebase
without requiring explicit null comparisons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
