HttpServerConnection: Don't spawn useless coroutines

[yhabteab]

Currently, for each Disconnect() call, we spawn a coroutine, but every one of them is just usesless, except the first one. However, since all Disconnect() usages share the same asio strand and cannot interfere with each other, spawning another coroutine within Disconnect() isn't even necessary. When a coroutine calls Disconnect() now, it will immediately initiate an async shutdown of the socket, potentially causing the coroutine to yield and allowing the others to resume. Therefore, the m_ShuttingDown flag is still required by the coroutines to be checked regularly.

[Al2Klimov]

The purpose of Atomic<> is not mixing atomic ops with not-atomic ones, including construction.

[Al2Klimov]

Suggested change

	std::atomic<bool> m_ShuttingDown;
	Atomic<bool> m_ShuttingDown;

[Al2Klimov]

As @julianbrost pointed out #10214 (comment), just using the default constructor of std::atomic is problematic. Atomic has no default constructor which enforces not (accidentally) using it (now and in future) at compile time (since #10215 🙈).

[yhabteab]

Are you serious? I can't even believe we're still talking about this now.

[Al2Klimov]

This is C++. If we can easily enforce something at compile time, we do it.

[yhabteab]

The purpose of Atomic<> is not mixing atomic ops with not-atomic ones, including construction.

Honestly, this class is just useless and I don't even understand why it is there in the first place. If std::atomic doesn't provide such a useless functionality, then it's for a good reason and you shouldn't try to work around it. I don't even understand why you are desperately trying to enforce its use here, as its extra functionality isn't actually necessary.

[Al2Klimov]

std::atomic doesn't provide such a useless functionality,

std::atomic<T>::atomic(T) isn't not atomic because of that's useless, but to achieve constexpr in constexpr std::atomic<T>::atomic(T) noexcept.

And it provides void std::atomic<T>::store(T) which programmers can use to initialize instances atomically not to mix (later) atomic ops with not-atomic ones (init).

And that's utilized by Atomic<T>::Atomic(T) – that's its purpose!

[julianbrost]

And it provides void std::atomic::store(T) which programmers can use to initialize instances atomically

Are you sure about that? https://en.cppreference.com/w/cpp/atomic/atomic/atomic:

The default-initialized std::atomic<T> does not contain a T object, and its only valid uses are destruction and initialization by std::atomic_init, see LWG issue 2334. (until C++20)

initialize instances atomically not to mix (later) atomic ops with not-atomic ones (init).

If that was indeed a problem, wouldn't this imply that std::atomic was completely broken? When would you think that would be a problem? If you initialize an std::atomic and wan't multiple threads to use it, that needs its own synchronization anyways (otherwise, the other thread could also call store() before the std::atomic was even constructed, which sounds very broken).

[Al2Klimov]

And it provides void std::atomic::store(T) which programmers can use to initialize instances atomically

Are you sure about that? https://en.cppreference.com/w/cpp/atomic/atomic/atomic:

The default-initialized std::atomic<T> does not contain a T object, and its only valid uses are destruction and initialization by std::atomic_init, see LWG issue 2334. (until C++20)

Damn! std::atomic<T>::atomic() is even less stable than std::atomic<T>::atomic(T), which at least "Initializes the underlying object with desired. The initialization is not atomic." One more reason for Atomic which enforces not using the default constructor!_{But see the below PR!🙈}

initialize instances atomically not to mix (later) atomic ops with not-atomic ones (init).

If that was indeed a problem, wouldn't this imply that std::atomic was completely broken?

No. As I said, (admittedly after std::atomic<T>::atomic(T) or std::atomic<T>::atomic(void) and std::atomic_init<T>()) one can use std::atomic<T>::store(T) (better ASAP) before any read(like) ops to absolutely guarantee latter will read a consistent value.

When would you think that would be a problem? If you initialize an std::atomic and wan't multiple threads to use it, that needs its own synchronization anyways

In the first place I think, the cost of std::atomic<T>::store(T), which Atomic uses, is not even measurable. So we can just use Atomic in all cases not even risking being wrong (with a false negative race condition), being always secured by std::atomic<T>::store(T).

• Feel free to revert c385355 if you're 10 billion percent sure. Otherwise, please just apply HttpServerConnection: Don't spawn useless coroutines #10214 (comment). Thanks.

[yhabteab]

In the first place I think, the cost of std::atomic<T>::store(T), which Atomic uses, is not even measurable. So we can just use Atomic in all cases not even risking being wrong (with a false negative race condition), being always secured by std::atomic<T>::store(T).

Can you please explain in a human readable way and not cryptically nor with a bunch of abbreviations what purpose you are trying to cover by using Atomic over std::atomic? With #10215 you just made the Atomic class even more useless by delegating to the non-atomic constructor of std::atomic and still calling its store() method on top of that. I'm not saying I don't want to use it because it's slow or anything, but it's pretty confusing to use such a class that has no real advantages over std::atomic.

• Feel free to revert c385355 if you're 10 billion percent sure. Otherwise, please just apply HttpServerConnection: Don't spawn useless coroutines #10214 (comment). Thanks.

No, I'm not going to do any of that. I haven't reviewed the PR it was introduced with, so it has nothing to do with this one, apart from the fact that I don't want to make use of it.

m_ShuttingDown doesn't use the std::atomic<T>::() constructor, so there's no need to replace it with Atomic class here. Change my mind!

icinga2/lib/remote/jsonrpcconnection.cpp

Line 42 in d894792

m_OutgoingMessagesQueued(io), m_WriterDone(io), m_ShuttingDown(false),

[Al2Klimov]

As @julianbrost pointed out #10214 (comment), just using the default constructor of std::atomic is problematic. Atomic has no default constructor which enforces not (accidentally) using it (now and in future) at compile time (since #10215 🙈).

[julianbrost]

As it felt like I had a déjà vu: #9991 actually did something quite similar, just to the JsonRpcConnection.

[julianbrost]

Speaking of spawning useless coroutines: is spawning a coroutine inside HttpServerConnection::Disconnect() even necessary at all? After a quick look, all calls to that method seem to be from coroutines running on the same strand, so shouldn't they be able to do all of this directly?

[julianbrost]

Oh and I totally forgot while typing that last comment: there are a few other changes (moving around checks of m_ShuttingDown) that are mentioned nowhere in the PR description or commit message. At first glance, I don't think they make things much better or worse, but if disconnects are triggered only from the same strand, there's probably little reason for these extra checks as that value then shouldn't change while a coroutine is running on that strand.

[Al2Klimov]

Speaking of spawning useless coroutines: is spawning a coroutine inside HttpServerConnection::Disconnect() even necessary at all? After a quick look, all calls to that method seem to be from coroutines running on the same strand, so shouldn't they be able to do all of this directly?

Can't this then stay a regular bool?

[Al2Klimov]

None of these seem to block any destructors. 👍

[julianbrost]

That method can now be made private and a quick doc comment for it stating that it must be called from a coroutine running on m_IoStrand would be very nice.

[Al2Klimov]

Aren't such above the function definitions and not the declarations like here?

[julianbrost]

Are both of you waiting for a reaction from the other one now? ;)

Aren't such above the function definitions and not the declarations like here?

On that topic: yes, that seems to often be the case in the Icinga 2 code. (Though I don't really understand why actually, so if you know, please enlighten me.)

[Al2Klimov]

"often" like there's a minority of locations like Yonas' one or like there's a minority(?) of locations w/o code doc at all?

[julianbrost]

"often" like I did not do any exact statistics. I don't know how many exceptions there are or if there are exceptions to that at all.