From 80362fa2c803ebd4dc78cc86b9d005450dc60798 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Tue, 19 May 2026 12:11:04 +0200 Subject: [PATCH] Freeze perfdata arrays and remove locks in code using them Since perfdata is set once when a check result is created and never changed again, locking this is unnecessary. This avoids components unnecessarily waiting on each other when processing perfdata. This fixes the locking cascade observed sometimes when the perfdata writer work queue blocks, where it extends to a lock on the entire check result eventually, affecting even more components. --- lib/icinga/checkresult.cpp | 6 ++++++ lib/icinga/checkresult.hpp | 1 + lib/icinga/pluginutility.cpp | 2 -- lib/methods/ifwapichecktask.cpp | 2 -- lib/perfdata/elasticsearchwriter.cpp | 1 - lib/perfdata/gelfwriter.cpp | 1 - lib/perfdata/graphitewriter.cpp | 1 - lib/perfdata/influxdbcommonwriter.cpp | 1 - lib/perfdata/opentsdbwriter.cpp | 1 - lib/perfdata/otlpmetricswriter.cpp | 1 - test/icinga-perfdata.cpp | 5 +++++ test/perfdata-perfdatawriterfixture.hpp | 6 +++--- test/utils.cpp | 7 ++++--- test/utils.hpp | 2 +- 14 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lib/icinga/checkresult.cpp b/lib/icinga/checkresult.cpp index f55b9025b..7629fe2e6 100644 --- a/lib/icinga/checkresult.cpp +++ b/lib/icinga/checkresult.cpp @@ -33,3 +33,9 @@ double CheckResult::CalculateLatency() const return latency; } + +void CheckResult::SetPerformanceData(const Array::Ptr& value) +{ + value->Freeze(); + ObjectImpl::SetPerformanceData(value); +} diff --git a/lib/icinga/checkresult.hpp b/lib/icinga/checkresult.hpp index b89f98d2a..fb0d7aa4d 100644 --- a/lib/icinga/checkresult.hpp +++ b/lib/icinga/checkresult.hpp @@ -22,6 +22,7 @@ public: double CalculateExecutionTime() const; double CalculateLatency() const; + void SetPerformanceData(const Array::Ptr& value); }; } diff --git a/lib/icinga/pluginutility.cpp b/lib/icinga/pluginutility.cpp index 9ebf901e1..cf8d320d0 100644 --- a/lib/icinga/pluginutility.cpp +++ b/lib/icinga/pluginutility.cpp @@ -185,8 +185,6 @@ String PluginUtility::FormatPerfdata(const Array::Ptr& perfdata, bool normalize) std::ostringstream result; - ObjectLock olock(perfdata); - bool first = true; for (const Value& pdv : perfdata) { if (!first) diff --git a/lib/methods/ifwapichecktask.cpp b/lib/methods/ifwapichecktask.cpp index 10fb3e67a..fb0a7e7b2 100644 --- a/lib/methods/ifwapichecktask.cpp +++ b/lib/methods/ifwapichecktask.cpp @@ -174,8 +174,6 @@ static void DoIfwNetIo( } if (perfdata) { - ObjectLock oLock (perfdata); - for (auto& pv : perfdata) { if (!pv.IsString()) { cr->SetOutput( diff --git a/lib/perfdata/elasticsearchwriter.cpp b/lib/perfdata/elasticsearchwriter.cpp index 9f5108ddd..ce79b6939 100644 --- a/lib/perfdata/elasticsearchwriter.cpp +++ b/lib/perfdata/elasticsearchwriter.cpp @@ -202,7 +202,6 @@ void ElasticsearchWriter::AddCheckResult(const Dictionary::Ptr& fields, const Ch CheckCommand::Ptr checkCommand = checkable->GetCheckCommand(); if (perfdata) { - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; diff --git a/lib/perfdata/gelfwriter.cpp b/lib/perfdata/gelfwriter.cpp index 6f8567f70..70ba70a55 100644 --- a/lib/perfdata/gelfwriter.cpp +++ b/lib/perfdata/gelfwriter.cpp @@ -196,7 +196,6 @@ void GelfWriter::CheckResultHandler(const Checkable::Ptr& checkable, const Check Array::Ptr perfdata = cr->GetPerformanceData(); if (perfdata) { - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; diff --git a/lib/perfdata/graphitewriter.cpp b/lib/perfdata/graphitewriter.cpp index e00cd9275..826429113 100644 --- a/lib/perfdata/graphitewriter.cpp +++ b/lib/perfdata/graphitewriter.cpp @@ -224,7 +224,6 @@ void GraphiteWriter::SendPerfdata(const Checkable::Ptr& checkable, const String& CheckCommand::Ptr checkCommand = checkable->GetCheckCommand(); - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; diff --git a/lib/perfdata/influxdbcommonwriter.cpp b/lib/perfdata/influxdbcommonwriter.cpp index 5b6f376de..1472c24e3 100644 --- a/lib/perfdata/influxdbcommonwriter.cpp +++ b/lib/perfdata/influxdbcommonwriter.cpp @@ -213,7 +213,6 @@ void InfluxdbCommonWriter::CheckResultHandler(const Checkable::Ptr& checkable, c double ts = cr->GetExecutionEnd(); if (Array::Ptr perfdata = cr->GetPerformanceData()) { - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; diff --git a/lib/perfdata/opentsdbwriter.cpp b/lib/perfdata/opentsdbwriter.cpp index 1b2f82a7d..6bc3001e8 100644 --- a/lib/perfdata/opentsdbwriter.cpp +++ b/lib/perfdata/opentsdbwriter.cpp @@ -308,7 +308,6 @@ void OpenTsdbWriter::AddPerfdata(const Checkable::Ptr& checkable, const String& CheckCommand::Ptr checkCommand = checkable->GetCheckCommand(); - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; diff --git a/lib/perfdata/otlpmetricswriter.cpp b/lib/perfdata/otlpmetricswriter.cpp index 5c60904be..136cdc4f0 100644 --- a/lib/perfdata/otlpmetricswriter.cpp +++ b/lib/perfdata/otlpmetricswriter.cpp @@ -181,7 +181,6 @@ void OTLPMetricsWriter::CheckResultHandler(const Checkable::Ptr& checkable, cons auto endTime = cr->GetExecutionEnd(); Array::Ptr perfdata = cr->GetPerformanceData(); - ObjectLock olock(perfdata); for (const Value& val : perfdata) { PerfdataValue::Ptr pdv; if (val.IsObjectType()) { diff --git a/test/icinga-perfdata.cpp b/test/icinga-perfdata.cpp index d1e7e75db..25516d886 100644 --- a/test/icinga-perfdata.cpp +++ b/test/icinga-perfdata.cpp @@ -38,6 +38,7 @@ BOOST_AUTO_TEST_CASE(quotes) BOOST_AUTO_TEST_CASE(multiple) { Array::Ptr pd = PluginUtility::SplitPerfdata("testA=123456 testB=123456"); + pd->Freeze(); BOOST_CHECK_EQUAL(pd->GetLength(), 2); String str = PluginUtility::FormatPerfdata(pd); @@ -47,12 +48,14 @@ BOOST_AUTO_TEST_CASE(multiple) BOOST_AUTO_TEST_CASE(multiline) { Array::Ptr pd = PluginUtility::SplitPerfdata(" 'testA'=123456 'testB'=123456"); + pd->Freeze(); BOOST_CHECK_EQUAL(pd->GetLength(), 2); String str = PluginUtility::FormatPerfdata(pd); BOOST_CHECK_EQUAL(str, "testA=123456 testB=123456"); pd = PluginUtility::SplitPerfdata(" 'testA'=123456 \n'testB'=123456"); + pd->Freeze(); BOOST_CHECK_EQUAL(pd->GetLength(), 2); str = PluginUtility::FormatPerfdata(pd); @@ -62,6 +65,7 @@ BOOST_AUTO_TEST_CASE(multiline) BOOST_AUTO_TEST_CASE(normalize) { Array::Ptr pd = PluginUtility::SplitPerfdata("testA=2m;3;4;1;5 testB=2foobar"); + pd->Freeze(); BOOST_CHECK_EQUAL(pd->GetLength(), 2); String str = PluginUtility::FormatPerfdata(pd, true); @@ -333,6 +337,7 @@ BOOST_AUTO_TEST_CASE(ignore_warn_crit_ranges) BOOST_AUTO_TEST_CASE(empty_warn_crit_min_max) { Array::Ptr pd = PluginUtility::SplitPerfdata("testA=5;;7;1;9 testB=5;7;;1;9 testC=5;;;1;9 testD=2m;;;1 testE=5;;7;;"); + pd->Freeze(); BOOST_CHECK_EQUAL(pd->GetLength(), 5); String str = PluginUtility::FormatPerfdata(pd, true); diff --git a/test/perfdata-perfdatawriterfixture.hpp b/test/perfdata-perfdatawriterfixture.hpp index e70b2123c..7813cc52b 100644 --- a/test/perfdata-perfdatawriterfixture.hpp +++ b/test/perfdata-perfdatawriterfixture.hpp @@ -63,7 +63,7 @@ object Host "h1" { void ReceiveCheckResults( std::size_t num, ServiceState state, - const std::function& fn = {} + const std::function& fn = {} ) { ::ReceiveCheckResults(m_Host, num, state, fn); @@ -95,8 +95,8 @@ object Host "h1" { auto start = std::chrono::steady_clock::now(); std::size_t unchangedCount = 0; while(true){ - ReceiveCheckResults(10, ServiceCritical, [&](const CheckResult::Ptr& cr) { - cr->GetPerformanceData()->Add(new PerfdataValue{GetRandomString("", 4096), 1}); + ReceiveCheckResults(10, ServiceCritical, [&](const CheckResult::Ptr&, const Array::Ptr& perfdata) { + perfdata->Add(new PerfdataValue{GetRandomString("", 4096), 1}); }); if (std::chrono::steady_clock::now() - start >= timeout) { diff --git a/test/utils.cpp b/test/utils.cpp index 95a9936cd..803cab4e1 100644 --- a/test/utils.cpp +++ b/test/utils.cpp @@ -93,7 +93,7 @@ void ReceiveCheckResults( const icinga::Checkable::Ptr& host, std::size_t num, icinga::ServiceState state, - const std::function& fn + const std::function& fn ) { using namespace icinga; @@ -114,12 +114,13 @@ void ReceiveCheckResults( Array::Ptr perfData = new Array; perfData->Add(new PerfdataValue{"dummy", 42}); - cr->SetPerformanceData(perfData); if (fn) { - fn(cr); + fn(cr, perfData); } + cr->SetPerformanceData(perfData); + BOOST_REQUIRE(host->ProcessCheckResult(cr, wg) == Checkable::ProcessingResult::Ok); } } diff --git a/test/utils.hpp b/test/utils.hpp index ec8b245d4..a18a3b7db 100644 --- a/test/utils.hpp +++ b/test/utils.hpp @@ -33,5 +33,5 @@ void ReceiveCheckResults( const icinga::Checkable::Ptr& host, std::size_t num, icinga::ServiceState state, - const std::function& fn = {} + const std::function& fn = {} );