Skip to content
This repository was archived by the owner on Jul 31, 2023. It is now read-only.

Commit f4ee744

Browse files
authored
Fix invalidation of MeasureDescriptors. (#399)
Heap-allocate MeasureDescriptors so that references to them are stable even if the underlying storage (std::vector) in the MeasureRegistry moves. Add test that fails under ASAN if this isn't done.
1 parent f7f7334 commit f4ee744

File tree

3 files changed

+26
-6
lines changed

3 files changed

+26
-6
lines changed

opencensus/stats/internal/measure_registry_impl.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include <iostream>
1818

19+
#include "absl/memory/memory.h"
1920
#include "opencensus/stats/internal/delta_producer.h"
2021
#include "opencensus/stats/internal/stats_manager.h"
2122
#include "opencensus/stats/measure_descriptor.h"
@@ -83,7 +84,8 @@ uint64_t MeasureRegistryImpl::RegisterImpl(MeasureDescriptor descriptor) {
8384
const uint64_t id =
8485
CreateMeasureId(registered_descriptors_.size(), true, descriptor.type());
8586
id_map_.emplace_hint(it, descriptor.name(), id);
86-
registered_descriptors_.push_back(std::move(descriptor));
87+
registered_descriptors_.push_back(
88+
absl::make_unique<MeasureDescriptor>(std::move(descriptor)));
8789
return id;
8890
}
8991

@@ -96,7 +98,7 @@ const MeasureDescriptor& MeasureRegistryImpl::GetDescriptorByName(
9698
MeasureDescriptor("", "", "", MeasureDescriptor::Type::kDouble);
9799
return default_descriptor;
98100
} else {
99-
return registered_descriptors_[IdToIndex(it->second)];
101+
return *registered_descriptors_[IdToIndex(it->second)];
100102
}
101103
}
102104

opencensus/stats/internal/measure_registry_impl.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#define OPENCENSUS_STATS_INTERNAL_MEASURE_REGISTRY_IMPL_H_
1717

1818
#include <cstdint>
19+
#include <memory>
1920
#include <string>
2021
#include <unordered_map>
2122
#include <vector>
@@ -74,9 +75,12 @@ class MeasureRegistryImpl {
7475
MeasureDescriptor::Type type);
7576

7677
mutable absl::Mutex mu_;
77-
// The registered MeasureDescriptors. Measure id are indexes into this
78-
// vector plus some flags in the high bits.
79-
std::vector<MeasureDescriptor> registered_descriptors_ GUARDED_BY(mu_);
78+
// The registered MeasureDescriptors. Measure ids are indexes into this
79+
// vector plus some flags in the high bits. Heap allocated so that the
80+
// descriptors themselves don't move around when the vector storage moves due
81+
// to resize.
82+
std::vector<std::unique_ptr<MeasureDescriptor>> registered_descriptors_
83+
GUARDED_BY(mu_);
8084
// A map from measure names to IDs.
8185
std::unordered_map<std::string, uint64_t> id_map_ GUARDED_BY(mu_);
8286
};
@@ -100,7 +104,7 @@ const MeasureDescriptor& MeasureRegistryImpl::GetDescriptor(
100104
MeasureDescriptor("", "", "", MeasureDescriptor::Type::kDouble);
101105
return default_descriptor;
102106
}
103-
return registered_descriptors_[IdToIndex(measure.id_)];
107+
return *registered_descriptors_[IdToIndex(measure.id_)];
104108
}
105109

106110
// static

opencensus/stats/internal/measure_registry_test.cc

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,20 @@ TEST(MeasureRegistryTest, GetMeasureByNameWithWrongTypeFails) {
147147
EXPECT_NE(measure_int.GetDescriptor(), measure_int_mistyped.GetDescriptor());
148148
}
149149

150+
TEST(MeasureRegistryTest, GrowDescriptorVector) {
151+
const std::string name = MakeUniqueName();
152+
ASSERT_TRUE(MeasureDouble::Register(name, "desc", "units").IsValid());
153+
const MeasureDescriptor& md = MeasureRegistry::GetDescriptorByName(name);
154+
155+
// Add descriptors until the vector of descriptors grows, moves, and
156+
// invalidates md above.
157+
for (int i = 0; i < 1000; ++i) {
158+
EXPECT_EQ(MeasureDescriptor::Type::kDouble, md.type());
159+
ASSERT_TRUE(
160+
MeasureDouble::Register(MakeUniqueName(), "desc", "units").IsValid());
161+
}
162+
}
163+
150164
} // namespace
151165
} // namespace stats
152166
} // namespace opencensus

0 commit comments

Comments
 (0)