Skip to content

Commit 129cb53

Browse files
committed
[yugabyte#30747] docdb: Fix nullptr crash in ThreadCategoryTracker::RegisterMetricEntity
Summary: **Issue:** we observed following error: ``` thread #1, name = 'yb-tserver', stop reason = signal SIGSEGV MetricPrototype::entity_type MetricEntity::CheckInstantiation MetricEntity::FindOrCreateMetric<FunctionGauge<...>> GaugePrototype::InstantiateFunctionGauge ThreadCategoryTracker::RegisterMetricEntity ThreadMgr::StartInstrumentation StartThreadInstrumentation RpcAndWebServerBase::RpcAndWebServerBase CQLServer::CQLServer TabletServerMain ``` `CheckInstantiation` was called with a null metric prototype, i.e. something in `entries_` had gauge_proto is nullptr. The cause is, scrape-time `GetCategory` acess `entries_` without any lock, so it could run while we were still registering that metric category. That lines up with the sequence below: 1. `RegisterGaugeForAllMetricEntities` wires `GetCategory` into the function gauges, callback is live(but `entries_` is not updated at this point) 2. A metrics scrape runs that callback `GetCategory`. `entries_[category].metric` insert a default row since category not exist yet (gauge proto null). 3. `RegisterMetricEntity` walks `entries_` and does `entry.gauge_proto->...` This hits the stub SIGSEGV. **Fix: ** wire FunctionGauge to only check the atomic value so scrapes only load the atomic, not entries_. That also removes concurrent std::map no lock access Test Plan: Jenkins Reviewers: sergei Reviewed By: sergei Subscribers: ybase, hsunder Differential Revision: https://phorge.dev.yugabyte.com/D51375
1 parent daba5a9 commit 129cb53

1 file changed

Lines changed: 18 additions & 17 deletions

File tree

src/yb/util/thread.cc

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545

4646
#include <algorithm>
4747
#include <array>
48+
#include <atomic>
4849
#include <functional>
4950
#include <map>
5051
#include <memory>
@@ -180,6 +181,10 @@ uint64_t GetInVoluntaryContextSwitches() {
180181
return ru.ru_nivcsw;
181182
}
182183

184+
uint64_t ReadAtomicInt(const std::atomic<uint64_t>* metric) {
185+
return metric->load(std::memory_order_relaxed);
186+
}
187+
183188
class ThreadCategoryTracker {
184189
public:
185190
explicit ThreadCategoryTracker(const string& name) : name_(std::move(name)) {}
@@ -189,12 +194,11 @@ class ThreadCategoryTracker {
189194
void RegisterMetricEntity(const scoped_refptr<MetricEntity> &metric_entity);
190195

191196
private:
192-
uint64 GetCategory(const string& category);
193-
uint64_t& RegisterGaugeForAllMetricEntities(const string& category);
197+
std::atomic<uint64_t>& RegisterGaugeForAllMetricEntities(const string& category);
194198

195199
struct Entry {
196200
std::unique_ptr<GaugePrototype<uint64>> gauge_proto;
197-
uint64_t metric = 0;
201+
std::atomic<uint64_t> metric{0};
198202
// We must retain references to each metric object from each metric_entity,
199203
// otherwise they will be cleaned up after 2 minutes.
200204
std::vector<scoped_refptr<FunctionGauge<uint64>>> metric_holders;
@@ -205,30 +209,28 @@ class ThreadCategoryTracker {
205209
std::vector<scoped_refptr<MetricEntity>> metric_entities_;
206210
};
207211

208-
uint64 ThreadCategoryTracker::GetCategory(const string& category) {
209-
return entries_[category].metric;
210-
}
211-
212212
void ThreadCategoryTracker::RegisterMetricEntity(const scoped_refptr<MetricEntity> &metric_entity) {
213213
for (auto& [category, entry] : entries_) {
214214
auto metric_ptr = entry.gauge_proto->InstantiateFunctionGauge(metric_entity,
215-
Bind(&ThreadCategoryTracker::GetCategory, Unretained(this), category));
215+
Bind(&ReadAtomicInt, Unretained(&entry.metric)));
216216
entry.metric_holders.push_back(std::move(metric_ptr));
217217
}
218218
metric_entities_.push_back(metric_entity);
219219
}
220220

221221
void ThreadCategoryTracker::IncrementCategory(const string& category) {
222-
++RegisterGaugeForAllMetricEntities(category);
222+
RegisterGaugeForAllMetricEntities(category).fetch_add(1, std::memory_order_relaxed);
223223
}
224224

225225
void ThreadCategoryTracker::DecrementCategory(const string& category) {
226-
--RegisterGaugeForAllMetricEntities(category);
226+
RegisterGaugeForAllMetricEntities(category).fetch_sub(1, std::memory_order_relaxed);
227227
}
228228

229-
uint64_t& ThreadCategoryTracker::RegisterGaugeForAllMetricEntities(const string& category) {
229+
std::atomic<uint64_t>& ThreadCategoryTracker::RegisterGaugeForAllMetricEntities(
230+
const string& category) {
230231
auto it = entries_.find(category);
231232
if (it != entries_.end()) {
233+
DCHECK(it->second.gauge_proto) << category;
232234
return it->second.metric;
233235
}
234236
auto id = name_ + "_" + category;
@@ -238,17 +240,16 @@ uint64_t& ThreadCategoryTracker::RegisterGaugeForAllMetricEntities(const string&
238240
std::make_unique<OwningGaugePrototype<uint64>>( "server", id, description,
239241
yb::MetricUnit::kThreads, description, yb::MetricLevel::kInfo, yb::EXPOSE_AS_COUNTER);
240242

241-
Entry entry;
243+
auto [inserted_it, inserted] = entries_.try_emplace(category);
244+
DCHECK(inserted) << category;
245+
Entry& entry = inserted_it->second;
242246
entry.gauge_proto = std::move(gauge_proto);
243247
for (auto& metric_entity : metric_entities_) {
244248
auto metric_ptr = entry.gauge_proto->InstantiateFunctionGauge(metric_entity,
245-
Bind(&ThreadCategoryTracker::GetCategory, Unretained(this), category));
249+
Bind(&ReadAtomicInt, Unretained(&entry.metric)));
246250
entry.metric_holders.push_back(metric_ptr);
247251
}
248-
249-
auto [inserted_it, inserted] = entries_.emplace(category, std::move(entry));
250-
DCHECK(inserted);
251-
return inserted_it->second.metric;
252+
return entry.metric;
252253
}
253254

254255
// A singleton class that tracks all live threads, and groups them together for easy

0 commit comments

Comments
 (0)