Replace legacy strand with executor-templated strand

[jschmidt-icinga]

This replaces boost::asio::io_context::strand with the executor-templated boost::asio::strand<boost::asio::io_context_executor_type>. This should prevent issues like #10825 to occur in the future, because the modern strand does not share implementation pointers between individual strand objects.

Detailed Description

The issue with the previously used strand was that there is a chance for two strand users (like a PerfdataWriterConnection and a JsonRpcConnection) to share an implementation object. This means that even though both objects hold distinct strand objects, one can block the execution of the other. So when one objects holds a lock, waits on an IO-Operation on that strand and the other object resumes on the strand and tries to acquire the same lock, both objects are deadlocked.

The executor-templated strand type is never shared between users and so should never run into this kind of deadlock. It's available for all boost versions we currently support and I've checked the changelog and git history carefully to see if there are any critical bugs that have been fixed since it was introduced. Technically boost-1.66 was the version this new strand has been introduced, but it seems there have been relatively few changes outside of some refactoring since then.

Currently this is all in one commit, because it's hard to switch this over gradually with some critical pieces (Timeout and GracefulDisconnect most notably) using the strand type explicitly and make incompatible calls on it which can't easily be generalized. And I didn't want to go through the effort of adding an overload either, just so it becomes pointless after switching everything over.

Notes

Use of std::shared_ptr<strand> instead of Shared<strand>::Ptr

When a Shared<Foo>::Ptr is dereferenced, the object is of type Shared that inherits from Foo. When passed to SpawnCoroutine(), this keeps older boost versions from using their dedicated boost::asio::spawn(const strand< Executor > & ex, ...) overloads and falls back to the generic Executor based overloads, which then fail somewhere in the machinery (the reason the overload exists).

Newer boost versions don't have this problem and the strand can just be treated as a generic executor, but for now we'll have to use std::shared_ptr<strand> for shared strands, because that correctly dereferences to the original type. The alternative would have been explicit object slicing with static_cast<strand&> at each call-site.

Honestly, I hadn't looked that closely at Shared<>::Ptr, but this makes it a giant ball of anti-patterns in my eyes. Most egregious of all, unconstrained deriving from arbitrary types isn't a good idea and might break all kinds of things. In my opinion we should consider deprecating it and just use std::shared_ptr with std::make_shared and std::shared_from_this in new code.

Closes #10861.

[Al2Klimov]

Why the std::shared_ptr?

[jschmidt-icinga]

Added a section to the PR description about the reason. In short, it's because on older boost versions boost::asio::spawn() don't know what to do with the Shared<strand>& it gets as a result of dereferencing the pointer.

[Al2Klimov]

In my opinion we should consider deprecating it and just use std::shared_ptr

Absolutely not. I introduced it specifically to save memory. We already had enough memory problems in the past, didn't we?

[jschmidt-icinga]

What memory does Shared<> save vs. std::shared_ptr<>? 🤔
It will save an additional allocation for the reference count in case someone doesn't use std::make_shared() as one should anyways. And in the rare cases where std::shared_from_this is actually needed, I think it stores an additional std::weak_ptr, but that's negligible for most our classes. At first glance I don't even see anything that would require the latter... we mostly use Shared<> for Stream objects, SSL context and things like that. std::shared_ptr should be perfectly fine for those, or am I missing something.

To be clear: I'm not saying deprecate boost::intrusive_ptr for our Object::Ptr, just shoehorning it into a generic shared pointer, which it was never meant to be.

[julianbrost]

Does that even need the external shared pointer? io_context::strand::strand explicitly documented a copy constructor where the copy shares the state, and strand<Executor> has a similar one, but unfortunately without explicit documentation, but from what I've seen in the code, it internally already uses a shared_ptr for its impl_ member (relevant code links: 1, 2, 3), so copies should still refer to the very same strand.

[jschmidt-icinga]

Right. I remember thinking about that at one point but tried to keep the changes minimal, and then I probably forgot about it again when I replaced Shared<> with std::shared_ptr<>. I'm happy to remove it entirely and just use the copy constructor in these cases.

[julianbrost]

This replaces the old boost::asio::io_context::strand with the more modern executor-templated boost::asio::strand<boost::asio::io_context_executor_type>.

Can you please share how to come to the same conclusion as you? The documentation for Boost 1.91.0 still mentions them as equal options:

An explicit strand is an instance of strand<> or io_context::strand.

The best I could find was this StackOverflow answer, but that's also more or less just "I looked at the code and came to that conclusion" without further references. I presume that's what you did as well, but can you suggest something to specifically look for in the implementations to come up with the same conclusion?

Apart from that, I quickly scrolled over the changes and they look plausible, but that's far from a proper review, but unless someone specifically says I should take a close look at this, I'd that isn't necessary.

[jschmidt-icinga]

Can you please share how to come to the same conclusion as you?

For one it is newer, much more flexible, embraces all the modern concepts, such as executors and has less technical debt (such as the described issue).

With the strand-sharing issue being the primary reason, I also wanted to make it clear that we're not falling back to some compatibility version, but to something that is more flexible and modern (in the context of ASIO). Note that I didn't claim io_context::strand was deprecated or that the boost maintainers try to discourage its use (which IMHO they should, or at least fix the sharing issue).

[julianbrost]

Looking into this turned out to be quite confusing. The history of both implementation files goes back to the beginning of the Git history in 2006 (I didn't bother to go further and try to dig out SVN history). And asio::strand was explicitly deprecated in Boost 1.57.0:

Explicitly marked asio::strand as deprecated. Use asio::io_service::strand instead.

However, that doesn't seem the refer to the current implementation of asio::strand. The deprecation was removed with boostorg/asio@b60e92b (the origin of that particular change seems to be chriskohlhoff/asio@b4bdfd1) which came with Boost 1.66:

Asio:

• Implemented interface changes to reflect the Networking TS (N4656).
  • See the list of new interfaces and, where applicable, the corresponding old interfaces that have been superseded.

Finally, that list, which is still present in the current version of the documentation, actually states that asio::strand is the newer implementation and the old one is deprecated:

In some cases the new Networking TS compatible interfaces supersede older Boost.Asio facilities. In these cases the older interfaces have been deprecated. The table below shows the new Networking TS interfaces and the facilities they replace:

New interface	Old interface	Notes
strand	io_service::strand	This template works with any valid executor, and is itself a valid executor.

Now, onto the shared implementation between different strand instances: This should be the corresponding part in asio::io_service::strand and while digging into all of this, I noticed something that looks surprisingly similar in asio::strand. Yes, that's only inside an #if, but that was only added very recently with boostorg/asio@3975462 and is only there starting with Boost 1.91.0, so at least depending on the Boost version, strands might still not be fully independent (I haven't looked into it in enough detail to know whether that mutex is actually held while executing handlers).

[julianbrost]

Hold on, the confusion got worse again... 🤯

Is there really both io_service::strand and io_context::strand? And worse: the text in the table and the link target differ. 😭

[jschmidt-icinga]

I noticed something that looks surprisingly similar in asio::strand

I was aware of this, and while it looks similar to what io_context::strand does, it is actually entirely different. Unlike the shared implementation objects, which essentially serialized the execution of handlers for all strand objects that share it, the shared mutex object is only used to protect the queuing of completion handler in the io service/context, to avoid race conditions. And as you note, even that is gone on recent enough boost and C++20.

Edit:
https://github.com/boostorg/asio/blob/4fa4abee89a62fdeeccac2585caece625f40647e/include/boost/asio/detail/impl/strand_executor_service.ipp#L106-L132

This is the most relevant section where those mutexes are used. You can see that nothing blocking happens while it is locked. All it does under the lock is update the state of the waiting and ready queues.

[jschmidt-icinga]

Is there really both io_service::strand and io_context::strand? And worse: the text in the table and the link target differ. 😭

Yes, but considering that io_service is actually deprecated (and AFAIK already removed since 1.87), that probably extends to io_service::strand as well. And it makes sense that io_service needed its own strand, given that these subclass strands aren't flexible with regards to the context/executor they use.

[julianbrost]

This replaces the old boost::asio::io_context::strand with the more modern executor-templated boost::asio::strand<boost::asio::io_context_executor_type>.

What do I need to look at to come up with the same conclusion. The more I try, the more confused I am.

If you git blame both boost::asio::io_context::strand and boost::asio::strand<boost::asio::io_context_executor_type> significant parts of both seem to come from 2017.

The problem with this PR is that the description sounds plausible, but doesn't provide any references backing it. So it it's incredibly hard for a reviewer to verify the claims. I mean I've been digging through Asio's documentation, source, and its Git history for quite a bit now and I'm still unsure what would be the preferred strand implementation and why.

Apart from this, the PR accumulated some merge conflicts in the meantime.

[jschmidt-icinga]

The problem with this PR is that the description sounds plausible, but doesn't provide any references backing it.

I can remove the "old" and "more modern" phrasing from the description if that helps you, but it's mostly irrelevant for the reason I made this PR. boost::asio::strand<> has zero disadvantages, is more flexible and most importantly fixes a limitation that has a realistic potential to bite us (again).

The "more modern" part is mostly inferred from the fact that it can work with any executor, which in my mind makes the "old" strand legacy, because what else besides compatibility with old code could be the reason to keep it around?

[julianbrost]

Older and more modern were claims where I was hoping that this would be easy to verify by checking the documentation. As this PR stands right now, I don't see how to do a proper review of it without basically having reviewing both Asio strand implementations.

[jschmidt-icinga]

Older and more modern were claims where I was hoping that this would be easy to verify

I mean technically both strands were added in 1.66, but only the templated one was really new, while the other was renamed from boost::asio::io_service::strand, which existed for a while before. So it is newer and more modern, and if it was any newer and more modern, we'd not be able to use it at all since we're still on 1.66 as our minimum version.

So your issue seems to be more along the lines that there is no clear "use this now" from the boost developers. Which I don't quite get, since there also isn't any "don't use this" for the templated strand. What remains is the concrete issue it solves for us.

And if you can't make that decision without auditing the entirety of both implementations, then that's what you'll have to do. I certainly didn't, I just verified that it solves the sharing issue. Or alternatively we can just leave things as they are and hope it doesn't break anything again (perhaps in an even more subtle and hard to debug way).