ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-26 23:19:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(insight): hold collector hooks weakly, and cover the lifetime

Browse Source 
callHooks() copied the hook list into a vector of raw pointers, released
mutex_, then dereferenced them. It has to release the lock: a handler may
drop the last reference to a hook, and ~OTelHookImpl re-acquires mutex_,
so invoking handlers under the non-recursive lock would deadlock. That
left a window in which an entry could be freed before it was used.

The window is not reachable today. Every hook belongs to a long-lived
ApplicationImp member, and onCollectionStopping() runs before those
members are destroyed, both from stop() and from the destructor body.
That call disarms each gauge via RemoveCallback, which blocks until an
in-flight callback finishes, because the SDK holds its registry mutex
across the callback. Safety therefore rests on four separate facts, none
of them enforced by a test, one of them internal to a vendored library.

Store weak references instead, so the code is correct by construction:
locking an entry keeps that hook alive for exactly its own handler call,
and a hook destroyed since the snapshot locks to null and is skipped.
Registration moves from the OTelHookImpl constructor to makeHook(),
because no weak_ptr to the object exists until the owning shared_ptr
does, and the destructor now prunes by expiry rather than by address.

Add three GTests over the real collector. A test-local MetricReader
drives one synchronous collection pass, since the SDK ships only a
threaded periodic reader. They assert a live hook runs, a destroyed hook
is skipped, and destroying one hook leaves its siblings registered --
the last pairing both directions so neither can pass vacuously.

Not yet run: verifying them needs a telemetry-enabled build of
xrpl_tests. Compile, clang-tidy and the pre-commit gates are clean.
This commit is contained in:
Pratik Mankawde
2026-09-10 15:39:58 +01:00
parent 366bcaa328
commit 859aafa3d6
2 changed files with 253 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
src/libxrpl/beast/insight/OTelCollector.cpp
View File

@@ -511,17 +511,25 @@ public:
/**
* @brief Register a hook for periodic invocation.
* @param hook Pointer to the hook to register.
*
* Takes the owning shared_ptr so the list can store a weak reference.
* Called from makeHook() rather than the hook's constructor, because a
* weak_ptr cannot be formed until the shared_ptr owns the object.
*
* @param hook Owning pointer to the hook to register.
*/
void
addHook(OTelHookImpl* hook);
addHook(std::shared_ptr<OTelHookImpl> const& hook);
/**
* @brief Unregister a hook.
* @param hook Pointer to the hook to unregister.
* @brief Drop entries for hooks that have been destroyed.
*
* Called from ~OTelHookImpl. The dying hook's weak_ptr has already
* expired by then, so the entry is identified by expiry rather than by
* address.
*/
void
removeHook(OTelHookImpl* hook);
removeExpiredHooks();
/**
* @brief Invoke all registered hooks.
@@ -597,8 +605,16 @@ private:
/**
* Registered hooks called during observable callbacks.
*
* Weak, not owning, and not raw. callHooks() must invoke handlers with
* mutex_ released, because a handler may drop the last reference to a
* hook and ~OTelHookImpl re-acquires mutex_. A raw pointer copied out of
* this list could therefore be dangling by the time it is dereferenced.
* Locking a weak_ptr instead keeps the hook alive for exactly the
* duration of its own handler call, and an already-destroyed hook is
* skipped rather than followed.
*/
std::vector<OTelHookImpl*> hooks_;
std::vector<std::weak_ptr<OTelHookImpl>> hooks_;
/**
* Registered gauges read during observable callbacks.
@@ -634,12 +650,14 @@ private:
OTelHookImpl::OTelHookImpl(HandlerType handler, std::shared_ptr<OTelCollectorImp> impl)
: impl_(std::move(impl)), handler_(std::move(handler))
{
impl_->addHook(this);
// Registration happens in OTelCollectorImp::makeHook(), not here: the
// list holds weak references, and no weak_ptr to this object exists
// until the owning shared_ptr does.
}
OTelHookImpl::~OTelHookImpl()
{
impl_->removeHook(this);
impl_->removeExpiredHooks();
}
void
@@ -849,7 +867,9 @@ OTelCollectorImp::~OTelCollectorImp()
Hook
OTelCollectorImp::makeHook(HookImpl::HandlerType const& handler)
{
return Hook(std::make_shared<OTelHookImpl>(handler, shared_from_this()));
auto hook = std::make_shared<OTelHookImpl>(handler, shared_from_this());
addHook(hook);
return Hook(hook);
}
Counter
@@ -883,17 +903,17 @@ OTelCollectorImp::makeMeter(std::string const& name)
}
void
OTelCollectorImp::addHook(OTelHookImpl* hook)
OTelCollectorImp::addHook(std::shared_ptr<OTelHookImpl> const& hook)
{
std::scoped_lock const lock(mutex_);
hooks_.push_back(hook);
hooks_.emplace_back(hook);
}
void
OTelCollectorImp::removeHook(OTelHookImpl* hook)
OTelCollectorImp::removeExpiredHooks()
{
std::scoped_lock const lock(mutex_);
std::erase(hooks_, hook);
std::erase_if(hooks_, [](std::weak_ptr<OTelHookImpl> const& hook) { return hook.expired(); });
}
void
@@ -910,15 +930,22 @@ OTelCollectorImp::callHooks()
// Copy the hook list under the lock, then invoke handlers outside it.
// A handler may drop the last reference to an OTelHookImpl, whose
// destructor calls removeHook() and re-acquires mutex_; invoking
// handlers while holding the (non-recursive) lock would deadlock.
std::vector<OTelHookImpl*> hooks;
// destructor re-acquires mutex_; invoking handlers while holding the
// (non-recursive) lock would deadlock.
std::vector<std::weak_ptr<OTelHookImpl>> hooks;
{
std::scoped_lock const lock(mutex_);
hooks = hooks_;
}
for (auto* hook : hooks)
hook->callHandler();
// Locking each entry keeps that hook alive across its own handler call,
// so releasing mutex_ above cannot leave a dangling reference. A hook
// destroyed since the snapshot was taken locks to null and is skipped.
for (auto const& weakHook : hooks)
{
if (auto const hook = weakHook.lock())
hook->callHandler();
}
}
void

208
src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp Normal file
View File

@@ -0,0 +1,208 @@
#ifdef XRPL_ENABLE_TELEMETRY
#include <xrpl/beast/insight/Collector.h>
#include <xrpl/beast/insight/Gauge.h>
#include <xrpl/beast/insight/Hook.h>
#include <xrpl/beast/insight/OTelCollector.h>
#include <xrpl/beast/utility/Journal.h>
#include <gtest/gtest.h>
#include <opentelemetry/metrics/meter_provider.h>
#include <opentelemetry/metrics/provider.h>
#include <opentelemetry/nostd/function_ref.h>
#include <opentelemetry/nostd/shared_ptr.h>
#include <opentelemetry/sdk/metrics/instruments.h>
#include <opentelemetry/sdk/metrics/meter_provider.h>
#include <opentelemetry/sdk/metrics/meter_provider_factory.h>
#include <opentelemetry/sdk/metrics/metric_reader.h>
#include <chrono>
#include <cstddef>
#include <memory>
#include <string>
#include <utility>
namespace beast::insight {
namespace metrics_api = opentelemetry::metrics;
namespace metrics_sdk = opentelemetry::sdk::metrics;
/**
* A MetricReader that collects only when the test asks it to.
*
* The SDK ships only PeriodicExportingMetricReader, whose background thread
* would make these tests depend on timing. MetricReader::Collect() is public
* and synchronous, so a minimal subclass lets a test drive one collection pass
* on the calling thread. That pass is what invokes an observable gauge's
* callback, which is the only path that reaches the collector's hooks.
*
* @code
* auto reader = std::make_shared<ManualMetricReader>();
* provider->AddMetricReader(reader);
* reader->collectOnce(); // runs every registered observable callback
* @endcode
*/
class ManualMetricReader : public metrics_sdk::MetricReader
{
public:
/**
* @brief Run exactly one collection pass, discarding the metric data.
*
* The tests assert on hook side effects, not on exported points, so the
* callback returns true without inspecting what it was handed.
*/
void
collectOnce()
{
Collect([](metrics_sdk::ResourceMetrics&) { return true; });
}
metrics_sdk::AggregationTemporality
GetAggregationTemporality(metrics_sdk::InstrumentType) const noexcept override
{
return metrics_sdk::AggregationTemporality::kCumulative;
}
bool
OnForceFlush(std::chrono::microseconds) noexcept override
{
return true;
}
bool
OnShutDown(std::chrono::microseconds) noexcept override
{
return true;
}
};
/**
* Installs a real SDK MeterProvider so observable gauges actually fire.
*
* OTelCollector takes its Meter from the global provider. Under the default
* noop provider an observable gauge's callback is never invoked, so a hook
* test would pass whatever the collector did. The fixture swaps in an SDK
* provider with a ManualMetricReader and restores the previous global provider
* afterwards, so it leaks no state into other telemetry tests in this binary.
*/
class OTelCollectorHooks : public ::testing::Test
{
protected:
void
SetUp() override
{
previous_ = metrics_api::Provider::GetMeterProvider();
reader_ = std::make_shared<ManualMetricReader>();
auto provider = metrics_sdk::MeterProviderFactory::Create();
provider->AddMetricReader(reader_);
provider_ = std::shared_ptr<metrics_sdk::MeterProvider>(std::move(provider));
metrics_api::Provider::SetMeterProvider(
opentelemetry::nostd::shared_ptr<metrics_api::MeterProvider>(provider_));
}
void
TearDown() override
{
metrics_api::Provider::SetMeterProvider(previous_);
provider_.reset();
reader_.reset();
}
/**
* @brief Build a collector, plus the armed gauge that drives its hooks.
*
* A collection pass only reaches the hooks through an observable gauge's
* callback, and a gauge is armed by onCollectionReady(), so every test
* needs both. The gauge is returned because dropping it would unregister
* the callback.
*
* Each test builds its own collector: the hook debounce is keyed to the
* time of the last invocation, which starts unset, so the first collection
* on a fresh collector always runs the hooks.
*/
static std::pair<Collector::ptr, Gauge>
makeArmedCollector()
{
auto collector = OTelCollector::New(
"http://127.0.0.1:4318/v1/metrics",
"",
"test-instance",
"xrpld",
"test",
Journal(Journal::getNullSink()));
auto gauge = collector->makeGauge("hook_test_gauge");
collector->onCollectionReady();
return {std::move(collector), std::move(gauge)};
}
opentelemetry::nostd::shared_ptr<metrics_api::MeterProvider> previous_;
std::shared_ptr<ManualMetricReader> reader_;
std::shared_ptr<metrics_sdk::MeterProvider> provider_;
};
// ---------------------------------------------------------------------------
// 1. A hook that is still alive runs on a collection pass.
// This is the registration path: makeHook() puts the hook on the
// collector's list, and an observable gauge callback invokes it. Without
// this, a hook that is never registered is indistinguishable from one that
// is registered and skipped.
// ---------------------------------------------------------------------------
TEST_F(OTelCollectorHooks, live_hook_runs_once_per_collection)
{
auto [collector, gauge] = makeArmedCollector();
std::size_t calls = 0;
auto const hook = collector->makeHook([&calls] { ++calls; });
reader_->collectOnce();
EXPECT_EQ(calls, 1u);
}
// ---------------------------------------------------------------------------
// 2. A hook destroyed before the collection pass is skipped, not called.
// The collector holds weak references, so the destroyed hook's entry locks
// to null. Asserting zero (not "did not crash") is what makes this a real
// check: a stale entry that was still followed would run the handler and
// increment the counter through freed memory.
// ---------------------------------------------------------------------------
TEST_F(OTelCollectorHooks, destroyed_hook_is_skipped)
{
auto [collector, gauge] = makeArmedCollector();
std::size_t calls = 0;
{
auto const hook = collector->makeHook([&calls] { ++calls; });
}
reader_->collectOnce();
EXPECT_EQ(calls, 0u);
}
// ---------------------------------------------------------------------------
// 3. Destroying one hook leaves its siblings registered.
// Guards the pruning step: removeExpiredHooks() erases by expiry rather
// than by address, so an over-broad predicate would drop live hooks too and
// silently stop their metrics updating.
// ---------------------------------------------------------------------------
TEST_F(OTelCollectorHooks, destroying_one_hook_keeps_the_others)
{
auto [collector, gauge] = makeArmedCollector();
std::size_t kept = 0;
std::size_t dropped = 0;
auto const keptHook = collector->makeHook([&kept] { ++kept; });
{
auto const droppedHook = collector->makeHook([&dropped] { ++dropped; });
}
reader_->collectOnce();
EXPECT_EQ(kept, 1u);
EXPECT_EQ(dropped, 0u);
}
} // namespace beast::insight
#endif // XRPL_ENABLE_TELEMETRY