Closed yhabteab closed 6 days ago
I'm not really sure about completely dropping the use of CpuBoundWork
in IfwApiCheckTask
without any replacement, in particular for calling Checkable::ProcessCheckResult()
. After all, that function does quite a lot, may lock different objects and so on.
An alternative could be moving this to the other thread pool, i.e. do it the same way how it's done to process regular check results. It uses the following callback where ProcessFinishedHandler()
calls ProcessCheckResult()
internally:
https://github.com/Icinga/icinga2/blob/4c6b93d61775ff98fc671b05ad4de2b62945ba6a/lib/methods/pluginchecktask.cpp#L51-L53
At the end, this callback will end up here:
https://github.com/Icinga/icinga2/blob/4c6b93d61775ff98fc671b05ad4de2b62945ba6a/lib/base/process.cpp#L1189-L1196
Regarding the individual commits and their messages:
Checkable::ExecuteCommandProcessFinishedHandler
has been utilised in an erroneous manner. Specifically, the variable is defined asthread_local
, yet the callback is executed by a different thread than the one that sets the actual callback, consequently resulting in the actual execute command handler being never called. To address this issue https://github.com/Icinga/icinga2/issues/10144, several functions have been altered to ensure thread safety, and the check results handlers are now executed correctly even from within a coroutine. Apart from that, in all instances where theCpuBoundWork
class was utilised, they just wasted costly semaphores, as they were either used to decode JSON responses or process check results, thereby impacting the functionality of the primary components, such as RPC and HTTP connections, that are more crucial than generating a dumb windows check result and processing countless, seemingly inconsequential boost signals. Consequently, this commit eliminates also all instances ofCpuBoundWork
class withinIfwApiCheckTask
, and delegates the process check result handling (as suggested in https://github.com/Icinga/icinga2/pull/10140#issuecomment-2321341579) to the global non-I/O thread pool instead.fixes #10144