From abf2fb392b1f9319323dab4241e3f83fd519b09c Mon Sep 17 00:00:00 2001 From: William Calliari <42240136+w1ll-i-code@users.noreply.github.com> Date: Mon, 10 Mar 2025 13:51:25 +0100 Subject: [PATCH 1/4] Keep object locked until events are dispatched. --- lib/icinga/checkable-check.cpp | 96 +++++++++++++++------------------- 1 file changed, 42 insertions(+), 54 deletions(-) diff --git a/lib/icinga/checkable-check.cpp b/lib/icinga/checkable-check.cpp index 38dca27ab4..4fc19d232b 100644 --- a/lib/icinga/checkable-check.cpp +++ b/lib/icinga/checkable-check.cpp @@ -104,10 +104,8 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr VERIFY(cr); VERIFY(producer); - { - ObjectLock olock(this); - m_CheckRunning = false; - } + ObjectLock olock(this); + m_CheckRunning = false; double now = Utility::GetTime(); @@ -169,8 +167,6 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr // This will be used to determine whether the on reachability changed event should be triggered. bool affectsPreviousStateChildren(reachable && AffectsChildren()); - ObjectLock olock(this); - CheckResult::Ptr old_cr = GetLastCheckResult(); ServiceState old_state = GetStateRaw(); StateType old_stateType = GetStateType(); @@ -333,8 +329,6 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr if (is_volatile && IsStateOK(old_state) && IsStateOK(new_state)) send_notification = false; /* Don't send notifications for volatile OK -> OK changes. */ - olock.Unlock(); - if (remove_acknowledgement_comments) RemoveAckComments(String(), cr->GetExecutionEnd()); @@ -350,25 +344,14 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr cr->SetVarsAfter(vars_after); - olock.Lock(); - + bool problem_change = false; if (service) { SetLastCheckResult(cr); } else { bool wasProblem = GetProblem(); - SetLastCheckResult(cr); - - if (GetProblem() != wasProblem) { - auto services = host->GetServices(); - olock.Unlock(); - for (auto& service : services) { - Service::OnHostProblemChanged(service, cr, origin); - } - olock.Lock(); - } + problem_change = GetProblem() != wasProblem; } - bool was_flapping = IsFlapping(); UpdateFlappingStatus(cr->GetState()); @@ -399,8 +382,6 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr } } - olock.Unlock(); - #ifdef I2_DEBUG /* I2_DEBUG */ Log(LogDebug, "Checkable") << "Flapping: Checkable " << GetName() @@ -411,36 +392,6 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr << "% current: " << GetFlappingCurrent() << "%."; #endif /* I2_DEBUG */ - if (recovery) { - for (auto& child : children) { - if (child->GetProblem() && child->GetEnableActiveChecks()) { - auto nextCheck (now + Utility::Random() % 60); - - ObjectLock oLock (child); - - if (nextCheck < child->GetNextCheck()) { - child->SetNextCheck(nextCheck); - } - } - } - } - - if (stateChange) { - /* reschedule direct parents */ - for (const Checkable::Ptr& parent : GetParents()) { - if (parent.get() == this) - continue; - - if (!parent->GetEnableActiveChecks()) - continue; - - if (parent->GetNextCheck() >= now + parent->GetRetryInterval()) { - ObjectLock olock(parent); - parent->SetNextCheck(now); - } - } - } - OnNewCheckResult(this, cr, origin); /* signal status updates to for example db_ido */ @@ -521,7 +472,6 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr * stash them into a notification types bitmask for maybe re-sending later. */ - ObjectLock olock (this); int suppressed_types_before (GetSuppressedNotifications()); int suppressed_types_after (suppressed_types_before | suppressed_types); @@ -551,6 +501,44 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr if ((stateChange || hardChange) && !children.empty() && (affectsPreviousStateChildren || AffectsChildren())) OnReachabilityChanged(this, cr, children, origin); + olock->Unlock(); + if (!service && problem_change) { + auto services = host->GetServices(); + for (auto& service : services) { + Service::OnHostProblemChanged(service, cr, origin); + } + } + + if (recovery) { + for (auto& child : children) { + if (child->GetProblem() && child->GetEnableActiveChecks()) { + auto nextCheck (now + Utility::Random() % 60); + + ObjectLock oLock (child); + + if (nextCheck < child->GetNextCheck()) { + child->SetNextCheck(nextCheck); + } + } + } + } + + if (stateChange) { + /* reschedule direct parents */ + for (const Checkable::Ptr& parent : GetParents()) { + if (parent.get() == this) + continue; + + if (!parent->GetEnableActiveChecks()) + continue; + + if (parent->GetNextCheck() >= now + parent->GetRetryInterval()) { + ObjectLock olock(parent); + parent->SetNextCheck(now); + } + } + } + return Result::Ok; } From 9d40de78eb426be2e15969cb256c8ab278c2e091 Mon Sep 17 00:00:00 2001 From: William Calliari <42240136+w1ll-i-code@users.noreply.github.com> Date: Wed, 16 Apr 2025 12:04:53 +0200 Subject: [PATCH 2/4] Address comments from review --- lib/icinga/checkable-check.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/icinga/checkable-check.cpp b/lib/icinga/checkable-check.cpp index 4fc19d232b..fe7b651bed 100644 --- a/lib/icinga/checkable-check.cpp +++ b/lib/icinga/checkable-check.cpp @@ -344,14 +344,21 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr cr->SetVarsAfter(vars_after); - bool problem_change = false; if (service) { SetLastCheckResult(cr); } else { bool wasProblem = GetProblem(); + SetLastCheckResult(cr); - problem_change = GetProblem() != wasProblem; + + if (GetProblem() != wasProblem) { + auto services = host->GetServices(); + for (auto& service : services) { + Service::OnHostProblemChanged(service, cr, origin); + } + } } + bool was_flapping = IsFlapping(); UpdateFlappingStatus(cr->GetState()); @@ -501,13 +508,7 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr if ((stateChange || hardChange) && !children.empty() && (affectsPreviousStateChildren || AffectsChildren())) OnReachabilityChanged(this, cr, children, origin); - olock->Unlock(); - if (!service && problem_change) { - auto services = host->GetServices(); - for (auto& service : services) { - Service::OnHostProblemChanged(service, cr, origin); - } - } + olock.Unlock(); if (recovery) { for (auto& child : children) { From a2c84398b77d10579d5f6837df4971129c846dea Mon Sep 17 00:00:00 2001 From: William Calliari <42240136+w1ll-i-code@users.noreply.github.com> Date: Tue, 22 Apr 2025 09:56:49 +0200 Subject: [PATCH 3/4] Add name to AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 0718440ad0..688a87eb86 100644 --- a/AUTHORS +++ b/AUTHORS @@ -45,6 +45,7 @@ Brian De Wolf Brian Dockter Bruno Lingner C C Magnus Gustavsson +Calliari William <42240136+w1ll-i-code@users.noreply.github.com> Carlos Cesario Carsten Köbke Chris Boot From 85ddb87c4106cd8adf5447d4a7034b4898e6bacb Mon Sep 17 00:00:00 2001 From: William Calliari <42240136+w1ll-i-code@users.noreply.github.com> Date: Thu, 8 May 2025 10:09:25 +0200 Subject: [PATCH 4/4] Fix Author --- AUTHORS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 688a87eb86..6b648735c5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -45,7 +45,6 @@ Brian De Wolf Brian Dockter Bruno Lingner C C Magnus Gustavsson -Calliari William <42240136+w1ll-i-code@users.noreply.github.com> Carlos Cesario Carsten Köbke Chris Boot @@ -302,6 +301,7 @@ vigiroux Vytenis Darulis Wenger Florian Will Frey +William Calliari <42240136+w1ll-i-code@users.noreply.github.com> Winfried Angele Wolfgang Nieder XnS