diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index c5daa449bf6..859620a5fa0 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -516,18 +516,20 @@ RecGetRecordCounter(const char *name, bool lock) RecErrT RecLookupRecord(const char *name, void (*callback)(const RecRecord *, void *), void *data, bool lock) { - RecErrT err = REC_ERR_FAIL; - ts::Metrics &metrics = ts::Metrics::instance(); - auto it = metrics.find(name); + RecErrT err = REC_ERR_FAIL; + ts::Metrics &metrics = ts::Metrics::instance(); + ts::Metrics::IdType metric_id; - if (it != metrics.end()) { - RecRecord r; - auto &&[name, type, val] = *it; + // A metric's storage is stable after creation. Avoid find()/end() here because end() is the current insertion position and + // can advance between those two calls while another thread registers a metric. + if (auto *metric = metrics.lookup(name, &metric_id); metric != nullptr) { + RecRecord r{}; r.rec_type = RECT_PLUGIN; - r.data_type = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; - r.name = name.data(); - r.data.rec_int = val; + r.data_type = metrics.type(metric_id) == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT; + r.name = name; + r.data.rec_int = metric->load(); + r.registered = true; callback(&r, data); err = REC_ERR_OKAY; diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 77d4c287625..cf749aa7a4c 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -17,11 +17,16 @@ or implied. See the License for the specific language governing permissions and limitations under the License. */ +#include +#include +#include + #include #include "records/RecCore.h" #include "iocore/eventsystem/EventSystem.h" #include "iocore/eventsystem/RecProcess.h" #include "tscore/Layout.h" +#include "tsutil/Metrics.h" #include "test_Diags.h" TEST_CASE("RecRegisterConfig - Type Dispatch", "[librecords][RecConfig]") @@ -87,3 +92,43 @@ TEST_CASE("RecRegisterStat - Type Dispatch", "[librecords][RecStat]") REQUIRE(value == 500); } } + +TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecLookup]") +{ + constexpr char record_name[] = "proxy.test.concurrent.string_value"; + constexpr char record_value[] = "stable"; + + REQUIRE(RecRegisterConfigString(RECT_CONFIG, record_name, record_value, RECU_DYNAMIC, RECC_NULL, nullptr, REC_SOURCE_NULL) == + REC_ERR_OKAY); + + std::atomic start{false}; + std::atomic finished{false}; + std::thread register_metrics([&]() { + while (!start.load(std::memory_order_acquire)) { + std::this_thread::yield(); + } + + for (int i = 0; i < 100000; ++i) { + auto metric_name = std::string{"proxy.process.test.concurrent_metric_registration."} + std::to_string(i); + + ts::Metrics::Counter::create(metric_name); + } + finished.store(true, std::memory_order_release); + }); + + bool all_lookups_succeeded = true; + size_t lookup_count = 0; + + start.store(true, std::memory_order_release); + while (!finished.load(std::memory_order_acquire)) { + if (RecGetRecordStringAlloc(record_name) != record_value) { + all_lookups_succeeded = false; + break; + } + ++lookup_count; + } + register_metrics.join(); + + CHECK(lookup_count > 0); + CHECK(all_lookups_succeeded); +}