chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4069208581
##########
src/bvar/variable.cpp:
##########
@@ -179,7 +201,14 @@ int Variable::expose_impl(const butil::StringPiece& prefix,
}
}
to_underscored_name(&_name, name);
-
+
+ bool expose_succeeded = false;
+ BUTIL_SCOPE_EXIT {
+ if (!expose_succeeded) {
+ _name.clear();
+ }
+ };
+
VarMapWithLock& m = get_var_map(_name);
Review Comment:
The rollback is not there to release a reservation. `_name` is built before
the map insert, so every failure return past that point has to put it back, and
in `MVariableBase::expose_impl()` the metric count cap returns early and used
to leave the mvar carrying a name it was never exposed under.
`Variable::expose_impl()` already did this with an explicit clear at the end of
the conflict path, the guard only makes it cover any later early return too.
The cross bvar and mbvar naming guarantee is deliberately not here, see the
reply on [mvariable.cpp](https://github.com/apache/brpc/pull/mvariable.cpp). It
lives in PrometheusMetricsDumper, which is the only place that sees both passes
and the synthesized names together, and keeping it out of expose() is what
makes this PR compatible with code that exposes such a pair today.
--
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]
