Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use thread-local var in coroutine & drop superfluous CpuBoundWork usage #10140

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Aug 28, 2024

  • c915949 - Acquiring a CPU slot just for traversing API user permissions without having to evaluate them is likewise just a wasted semaphore, thus this commit drops its usage.
  • 74009f0 - It appears that the Checkable::ExecuteCommandProcessFinishedHandler has been utilised in an erroneous manner. Specifically, the variable is defined as thread_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 IfwApiCheckTask uses thread-local variable in coroutine #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 the CpuBoundWork 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 of CpuBoundWork class within IfwApiCheckTask, and delegates the process check result handling (as suggested in Don't use thread-local var in coroutine & drop superfluous CpuBoundWork usage #10140 (comment)) to the global non-I/O thread pool instead.

fixes #10144

@yhabteab yhabteab added enhancement New feature or request core/quality Improve code, libraries, algorithms, inline docs labels Aug 28, 2024
@yhabteab yhabteab added this to the 2.15.0 milestone Aug 28, 2024
@cla-bot cla-bot bot added the cla/signed label Aug 28, 2024
@yhabteab yhabteab changed the title IfwApiCheckTask: Drop superfluous CpuBoundWork usage Drop superfluous CpuBoundWork usage again Aug 28, 2024
@julianbrost
Copy link
Contributor

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:

callback = [checkable, cr](const Value& commandLine, const ProcessResult& pr) {
ProcessFinishedHandler(checkable, cr, commandLine, pr);
};

At the end, this callback will end up here:

icinga2/lib/base/process.cpp

Lines 1189 to 1196 in 4c6b93d

if (m_Callback) {
/*
* Explicitly use Process::Ptr to keep the reference counted while the
* callback is active and making it crash safe
*/
Process::Ptr process(this);
Utility::QueueAsyncCallback([this, process]() { m_Callback(m_Result); });
}

@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch 2 times, most recently from a88f2e1 to 0aa76ff Compare August 30, 2024 18:40
@yhabteab yhabteab self-assigned this Sep 2, 2024
@yhabteab yhabteab added bug Something isn't working and removed enhancement New feature or request core/quality Improve code, libraries, algorithms, inline docs labels Sep 2, 2024
@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch 4 times, most recently from 1ec4c04 to e866554 Compare September 4, 2024 06:55
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch from e866554 to eaf923d Compare September 4, 2024 13:15
@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch from eaf923d to 1f3d7da Compare September 5, 2024 13:12
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch from 1f3d7da to 04e085c Compare September 5, 2024 14:56
@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch from 04e085c to e24511d Compare September 5, 2024 14:58
@julianbrost
Copy link
Contributor

Regarding the individual commits and their messages:

  • e24511d: Maybe something went wrong here during a rebase, but I don't really see how those two individual changes belong together
  • 4f5a915: The message doesn't really describe the changes. I think the message is supposed to say that if fixes IfwApiCheckTask uses thread-local variable in coroutine #10144, but it does more than that and also, I'd say "thread-safe" isn't really the right word here, that's typically used when referring multiple threads accessing something in parallel (here it's just a logic error that if a job is moved to a different thread, it probably shouldn't rely on a thread-local variable).

@yhabteab yhabteab force-pushed the drop-cpu-bound-work-usage-from-ifwapi branch from e24511d to 74009f0 Compare September 5, 2024 15:36
@yhabteab yhabteab changed the title Drop superfluous CpuBoundWork usage again Don't use thread-local var in coroutine & drop superfluous CpuBoundWork usage Sep 5, 2024
lib/remote/httpserverconnection.cpp Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Show resolved Hide resolved
lib/methods/ifwapichecktask.cpp Show resolved Hide resolved
@yhabteab yhabteab added the consider backporting Should be considered for inclusion in a bugfix release label Sep 6, 2024
lib/methods/ifwapichecktask.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed consider backporting Should be considered for inclusion in a bugfix release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IfwApiCheckTask uses thread-local variable in coroutine
3 participants