Copilot commented on code in PR #3487:
URL: https://github.com/apache/brpc/pull/3487#discussion_r3845447863
##########
src/bvar/detail/sampler.cpp:
##########
@@ -211,9 +223,44 @@ void Sampler::schedule() {
}
void Sampler::destroy() {
- _mutex.lock();
+ BAIDU_SCOPED_LOCK(_mutex);
_used = false;
- _mutex.unlock();
+ if (_nborrow > 0) {
+ // The owning bvar is being destructed while Window/PerSecond objects
+ // still borrow this sampler. Leak the sampler so that the borrowers
+ // keep pointing at valid memory (they just stop getting new samples),
+ // which turns a use-after-free into a bounded leak.
+ _leaked = true;
+ std::string owner =
+ _debug_name.empty() ? "An unnamed bvar" : "bvar ";
+ if (_debug_name.empty()) {
Review Comment:
The condition building `owner` is inverted, so named bvars won’t print their
name (and unnamed bvars append empty quotes). This breaks the diagnostic that’s
meant to identify the offending bvar. Flip the condition to append the quoted
name when `_debug_name` is not empty, and keep the unnamed case unmodified.
##########
src/bvar/detail/sampler.cpp:
##########
@@ -211,9 +223,44 @@ void Sampler::schedule() {
}
void Sampler::destroy() {
- _mutex.lock();
+ BAIDU_SCOPED_LOCK(_mutex);
_used = false;
- _mutex.unlock();
+ if (_nborrow > 0) {
+ // The owning bvar is being destructed while Window/PerSecond objects
+ // still borrow this sampler. Leak the sampler so that the borrowers
+ // keep pointing at valid memory (they just stop getting new samples),
+ // which turns a use-after-free into a bounded leak.
+ _leaked = true;
+ std::string owner =
+ _debug_name.empty() ? "An unnamed bvar" : "bvar ";
+ if (_debug_name.empty()) {
+ owner.append("'").append(_debug_name).append("'");
+ }
+ if (FLAGS_bvar_abort_on_sampler_still_borrowed) {
+ LOG(FATAL) << "Abort because " << owner << " is destructed while "
+ << _nborrow << " Window/PerSecond still reference its"
+ " sampler";
+ } else {
+ LOG(ERROR) << owner << " is destructed while " << _nborrow
+ << " Window/PerSecond still reference its sampler. The"
+ " bvar referenced by a Window MUST be destructed"
+ " AFTER that Window, see comments of Window in"
+ " bvar/window.h. The sampler is leaked to avoid a"
+ " dangling pointer.";
+ }
+ }
Review Comment:
Logging (and potentially `LOG(FATAL)`) is done while holding
`Sampler::_mutex`. This increases lock hold time and can amplify contention
between the sampling thread and destroy/remove_borrower paths. Consider
capturing `_nborrow`, `_debug_name`, and setting `_used/_leaked` under the
lock, then releasing the lock before performing the `LOG(...)` calls.
--
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]
