Checkable#ProcessCheckResult(): discard🗑️ CR or delay its producers shutdown

[Al2Klimov]

This PR is the first step in the process of making the shutdown order of our components less unpredictable. Specifically, the goal is to ensure that components that produce any kind of events (e.g. notifications) or check results are stopped in a determined order, and they don't cause any post-stop events to be triggered. The problem this PR specifically addresses is that e.g. stopping the CheckerComponent does prevent new checks from being scheduled, but it does not wait for already started (so can't be cancelled) checks to finish. This can lead to a situation where the e.g. IcingaDB component is stopped before such checks complete and causing all the resulted events to be lost, because IcingaDB is not able to process them anymore. How does this PR solve this?

It introduces a new CheckResultProducer interface that is implemented only by types that actually produce check results, which as of now are CheckerComponent (for local checks) and ApiListener (for remote checks). Each of such instance maintains a simple counter of the number of ongoing checks results it produces. However, they don't just increment the counter after e.g. Checkable::ExecuteCheck() is called, but only after the check execution produces a result and enters the Checkable::ProcessCheckResult() method, and decremented when leaving it. This way, we don't have to worry about the fact that check execution time can be very long and block the shutdown of the component indefinitely (credit goes to @Al2Klimov). Ok, but how does this prevent triggering post-factum events, you ask? Well, the Checkable::ProcessCheckResult requires now the CheckResultProducer be passed in as an argument, and is only allowed to actually process that result after locking the producer, otherwise the cr is discarded.

But what does locking the producer mean? It doesn't mean acquiring the ObjectLock on that object, but rather acquiring a shared lock¹ on it. However, here's the catch: the producer is only shared lockable if its Stop() method hasn't been already called. Meaning, whenever CheckerComponent::Stop() is called, any thread that tries to acquire a shared lock on it in Checkable::ProcessCheckResult() will ultimately fail and discard the cr without ever processing it. On the other hand, if the checker was stopped after some threads have successfully acquired a shared lock on it (remember that when a thread acquires a shared lock, the producer automatically increments its counter), it will wait for all of them to finish while preventing any new threads from acquiring the lock until its counter goes back to 0.

The only remaining problem that this PR doesn't solve is that ApiListener::Stop() implementation doesn't gracefully disconnect all active connections, which means that we might still lose some important check results received from remote endpoints. However, since this is a completely different issue, it will be addressed in a separate PR.

Edit (yhabteab) end:

This is the very first and hardest step of the #10179 (comment) plan.

Checkable#ProcessCheckResult() now takes a CheckResultProducer::Ptr:

• 23c2365 Checkable#ProcessCheckResult(): discard CR or delay its producers shutdown

It tries to shared-lock the CheckResultProducer:

• Failure means the CheckResultProducer indicates it's already shutting down, so the not yet really started Checkable#ProcessCheckResult() fails and discards the CR
• Success means Checkable#ProcessCheckResult() can continue and on shutdown CheckResultProducer will wait for that

This implies that every component producing checks needs to implement CheckResultProducer:

• f86b8ba *#Stop(): wait for own Checkable#ProcessCheckResult()s to finish
• 84cb9d8 mkclass: inherit from Object like this: class T : virtual public Object

Finally, the CheckResultProducer::Ptr has "just" to be passed through from the CheckResultProducer itself to Checkable#ProcessCheckResult():

Footnotes

1. The new CheckResultProducer interface satisfies the std::SharedLockable requirements. ↩

[julianbrost]

• TODO(@Al2Klimov): better PR description

Well, that's indeed necessary.

@julianbrost rather agreed with #10179 (comment), but said #10179 (comment) lacks some tracking.

Interesting that I've said something about a comment I haven't even seen until just now.

[Al2Klimov]

@julianbrost Very funny. Fact is, on Thursday you

1. agreed with me that components should stop/join their own checks along with themselves
2. said that will need some tracking

So I've done that. So far:

• 684ca7d Now Checkable#ProcessCheckResult() doesn't guess the CR origin, instead it demands a CheckResultProducer barely fitting into a std::shared_lock
• a8d011f CheckerComponent implements that CheckResultProducer (TODO(@Al2Klimov): more classes)

Finally, CheckResultProducer::Ptr just has to be passed through.

[yhabteab]

@Al2Klimov please don't be mad at me :)! I took the initiative to expand the PR description slightly based on the analysis made together with @jschmidt-icinga, so everyone can follow here without having to browse through the changed files. Though, you can gladly merge all the commits suffixed with the take CheckResultProducer::Ptr message into one commit with a meaningful commit message. They're basically all the same, just for different classes and clutter the PR unnecessarily and make it appear larger than it is and cryptic.

[jschmidt-icinga]

I think in principle this solves the problem well, but I agree with @yhabteab, kindly squash it down to a single commit describing the functional change (the PR title is fine), as none of the individual changes make sense without all the others. That would also make it much easier to review on GH.

[Al2Klimov]

Are you sure? I've squashed the low-hanging fruits into 170e7cf – and see how big this one commit became!

[jschmidt-icinga]

Are you sure? I've squashed the low-hanging fruits into 170e7cf – and see how big this one commit became!

Yes, I'm sure. There's big commits and there's commits with trivial one-line changes in lots of files. You could split off adding the new Classes, maybe adding the final check, but at least all the function signature changes should be in a single commit.

What's more, commits should reflect individual consistent states of the code, i.e., each of them, when checked out, should at least still compile and the program should still work. Granted this is less important when on a branch that gets merged, but its still a good idea to maintain some discipline.

[jschmidt-icinga]

I've had a discussion with @julianbrost and @yhabteab with the following results:

• We'd like to avoid virtual inheritance by using the new class by composition as a member instead of inheritance. While nobody is strictly against it, we don't see the benefit of using it in this situation over just keeping and passing a class member.
• The class should then derive from SharedObject instead of Object because it doesn't need any of Object's capabilities, just the ::Ptr.
• It can be named "StoppableWaitGroup" to reflect the (apparent) similarity with Go WaitGroups, with the difference that a stop can be signaled. We are all satisfied with that name but still open to other suggestions.
• Further on that note, the initialization done in Start() can be done in the constructor (which basically means just initializing m_InstanceIsActive to true instead of false) and Stop() should be named Wait().
• A lock_shared() method should be implemented to properly implement the SharedLockable interface, even if it is not used at the moment. As per the spec, it should throw when no lock can be obtained. Internally it can use try_lock_shared() and just throw when that returns false.

[Al2Klimov]

avoid virtual inheritance by using the new class by composition as a member instead of inheritance. While nobody is strictly against it, we don't see the benefit of using it

Benefits of my version (ordered descending by importance):

• It's cool af 😎
• It perfectly restricts who can exclusive-lock (inheritors) and who is allowed to shared-lock (everyone) – I mean...

to reflect the (apparent) similarity with Go WaitGroups,

... especially when you talk about Go, remember how context works:

• context.Context can only query some info and wait
• only context.Context creator can cancel it

This restriction would vanish here if I got your idea correctly:

• Getters for all affected component classes
• A getter returns (a pointer to) the whole thing, so everyone can call everything

(Given #10397 (comment), strange that you wanna give up such a restriction.)

Let me know whether my concern is legit or I shall refactor anyway.

the initialization done in Start() can be done in the constructor (which basically means just initializing m_InstanceIsActive to true instead of false)

Before the component is activated? But why?

[jschmidt-icinga]

... especially when you talk about Go, remember how context works:

* context.Context can only query some info and wait

* only context.Context creator can cancel it

That issue is already solved when you take into account that we pass the base class pointer that doesn't have a virtual interface for the Stop() method.

This restriction would vanish here if I got your idea correctly:

* Getters for all affected component classes

* A getter returns (a pointer to) the whole thing, so everyone can call everything

Maybe we're talking past each other a bit. Just to clarify, the intention is not to get rid of passing what's currently the CheckResultProducer::Ptr down the line to ProcessCheckResult(). That can stay exactly the same. If that was the misunderstanding, I can understand your reaction.

Otherwise, "all affected" is doing some heavy lifting here, when it's only ApiListener that would need a getter, and that only due to the situation that other sources outside ApiListener have to now use an ApiListener instance to do their ExecuteRemoteChecks and direct ProcessCheckResults. All other components can just use their private member object privately.

Something like ApiListener::GetInstance()->GetWaitGroup() would return a pointer to the base class (Maybe just WaitGroup 🤷 ) that only has the lock semantics, not the Wait() interface. On that note, if you really want to make this similar to Go WaitGroups plus Context and immediately more flexible for no additional effort, the derived StoppableWaitGroup would also have a Cancel() method that just sets the bool, that we could call before or after Wait() depending on the use case. No need to iron that out here though, since it isn't relevant for this PR, and can easily be added when needed.

(Given #10397 (comment), strange that you wanna give up such a restriction.)

Let me know whether my concern is legit or I shall refactor anyway.

In addition to what I wrote above, with SharedObject being used instead of Object and no inheritance, there's a lot less casting magic that could be done with this pointer.

Also that is about access control, which is an aspect of C++ I don't necessarily care that much about, while in the linked comment I was talking about implementation hiding, which is about not having to know how the sausage is made, both as a compiler including the header, and a programmer looking at a header to learn about how to use a class. I get that currently this is an annoyance in C++ and requires a lot of discipline by the programmer and is not always worth the effort. Modules will improve that, when they are ready to be used some time around 2070.

Before the component is activated? But why?

Because why not. It makes the class simpler, with fewer possible user errors, and for the immediate use case of controlling check result ownership it doesn't matter, because a component that hasn't started yet will not order any checks.

[Al2Klimov]

• The class should then derive from SharedObject instead of Object because it doesn't need any of Object's capabilities, just the ::Ptr.

I'm afraid Object is required:

/Users/aklimov/NET/WS/boost_1_69_0/boost/smart_ptr/intrusive_ptr.hpp:302:12: error: static_cast from 'icinga::WaitGroup *' to 'icinga::Object *', which are not related by inheritance, is not allowed
  302 |     return static_cast<T *>(p.get());
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/aklimov/NET/WS/icinga2/lib/base/value.hpp:59:11: note: in instantiation of function template specialization 'boost::static_pointer_cast<icinga::Object, icinga::WaitGroup>' requested here
   59 |                 : Value(static_pointer_cast<Object>(value))
      |                         ^
/Users/aklimov/NET/WS/icinga2/lib/icinga/checkcommand.cpp:19:3: note: in instantiation of function template specialization 'icinga::Value::Value<icinga::WaitGroup>' requested here
   19 |                 producer,
      |                 ^
1 error generated.

• Further on that note, the initialization done in Start() can be done in the constructor (which basically means just initializing m_InstanceIsActive to true instead of false) and Stop() should be named Wait().

I like Join() better tbh.

• A lock_shared() method should be implemented to properly implement the SharedLockable interface, even if it is not used at the moment. As per the spec,

"As per the spec," we may only use #ifndef for include guards. But we use #pragma once nowadays, because we can, and I'm proud of it.

Seriously speaking, it feels pretty cringe to implement a method we know in advance nobody's gonna use AND that the method won't do anything useful.

[jschmidt-icinga]

I'm afraid Object is required:

Right, because we want to store it in a value for Function::Invoke().

I think there is a way to still do this by extending Value to contain SharedObjects and using std::enable_if, std::is_base_of instead of the static_assert in value.hpp, but that's way past the scope of this PR. We can use Object for the time being.

I like Join() better tbh.

I agree, use Join().

Seriously speaking, it feels pretty cringe to implement a method we know in advance nobody's gonna use AND that the method won't do anything useful.

In fact, I already have a use for it shutting down HttpHandler::ProcessRequest(). Throwing instead of returning is much better in that situation. But it's ok, you can leave it out for now, I'll add it in my own PR when it is actually going to be used.

Edit: Only just realized you already pushed the update. I'll have another look right away.

[jschmidt-icinga]

Looks good to me now. I really think this is a great addition. 👏

But maybe lets hold back on merging until Monday so @julianbrost can have another quick look.

[jschmidt-icinga]

Just one more quick remark: By accident you seem to have used spaces for indentation on the new files.

[yhabteab]

There were some (specifically two ;)) minor changes made since your last review, so please have another look again!