Always use Configuration#Concurrency, not std::thread::hardware_concurrency()

[Al2Klimov]

refs #7842
fixes #9147

[Al2Klimov]

On its own from #7845. You said this is an improvement by itself.

[julianbrost]

Making Post() implicitly start the threads compared to an explicit Start() looks fragile. You wouldn't really notice a call to Post() that happens too early (before Configuration::Concurrency was set) which would mean the pool could potentially still be initialized with the incorrect size.

Or worse, a Stop(); Post(); fork(); sequence would accidentally start the threads again. Given that you could change the return type of Post() from bool to void, it looks like no caller is checking the return value, which should correspond to expecting this call to happen in between Stop(); and Start();.

Anyways, if this PR is correct, it would also be correct to delay the initialization of the thread pool until after Configuration::Concurrency was set. And this sounds more robust to me (maybe even make Post() throw an exception on uninitialized pools, at least in debug builds) as you can't get a surprise pool initialization with that approach. How thoroughly did you go to Post() calls to make sure none of the existing ones may happen too early?

[Al2Klimov]

What about the following?

1. Initial concurrency: 0 which is effectively 1
2. This way Application::InitializeBase() starts a minimal TP not running into Icinga 2 hangs w/o error message when running into cgroup limits #9099
3. Once Configuration#Concurrency may have been set (before config commit?) if it's still 0:
   • set a sane default (std::thread::hardware_concurrency() for now)
   • restart TP

[julianbrost]

With that suggestion, would you have an explicit Start() method? And this somewhat sounds like you would start a temporary thread pool that shouldn't be used, just to tear it down again later.

[Al2Klimov]

1. Yes, an explicit Start() method.
2. One can use it. But yes, it will be reset on concurrency change.

[Al2Klimov]

Re: #9643 (comment)

Just saw that the I/O engine already has the same "problem". So this is fine.

[julianbrost]

Just saw that the I/O engine already has the same "problem".

You mean it starts the threads automatically on demand?

[Al2Klimov]

Exactly, that's why it's lazy-inited.

[julianbrost]

Wait, I just noticed that you haven't changed anything at all here.

Just saw that the I/O engine already has the same "problem".

If component A has an undesired property and component B doesn't, obviously the way forward is to also introduce that undesired property in component B. Well, actually, it isn't.

[Al2Klimov]

you would start a temporary thread pool that shouldn't be used, just to tear it down again later.

Not exactly. ThreadPool#Post() is called by Utility::QueueAsyncCallback() which is called by Timer::TimerThreadProc() due to including but not limited to WorkQueue#WorkQueue() used by ConfigItem::RunWithActivationContext() which is called pre-commit. Therefore I'll do #9643 (comment) .

[Al2Klimov]

$ cat hardware_concurrency.conf
log(Concurrency)
$ prefix/sbin/icinga2 daemon -c hardware_concurrency.conf
[2023-05-11 17:06:24 +0200] information/cli: Icinga application loader (version: v2.11.0-527-g6e2b68789; debug)
[2023-05-11 17:06:24 +0200] information/cli: Loading configuration file(s).
[2023-05-11 17:06:24 +0200] information/config: 8
[2023-05-11 17:06:24 +0200] information/ConfigItem: Committing config item(s).
[2023-05-11 17:06:24 +0200] information/ConfigItem: Instantiated 1 IcingaApplication.
[2023-05-11 17:06:24 +0200] information/ScriptGlobal: Dumping variables to file '/Users/aklimov/NET/WS/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2023-05-11 17:06:24 +0200] information/ConfigItem: Triggering Start signal for config items
[2023-05-11 17:06:24 +0200] information/ConfigItem: Activated all objects.
^C[2023-05-11 17:06:29 +0200] information/Application: Received request to shut down.
[2023-05-11 17:06:29 +0200] information/Application: Shutting down...
[2023-05-11 17:06:29 +0200] information/ConfigObject: Dumping program state to file '/Users/aklimov/NET/WS/icinga2/prefix/var/lib/icinga2/icinga2.state'
[2023-05-11 17:06:29 +0200] information/IcingaApplication: Icinga has shut down.
$ prefix/sbin/icinga2 daemon -DConfiguration.Concurrency=9 -c hardware_concurrency.conf
[2023-05-11 17:06:53 +0200] information/cli: Icinga application loader (version: v2.11.0-527-g6e2b68789; debug)
[2023-05-11 17:06:53 +0200] information/cli: Loading configuration file(s).
[2023-05-11 17:06:53 +0200] information/config: 9
[2023-05-11 17:06:53 +0200] information/ConfigItem: Committing config item(s).
[2023-05-11 17:06:53 +0200] information/ConfigItem: Instantiated 1 IcingaApplication.
[2023-05-11 17:06:53 +0200] information/ScriptGlobal: Dumping variables to file '/Users/aklimov/NET/WS/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2023-05-11 17:06:53 +0200] information/ConfigObject: Restoring program state from file '/Users/aklimov/NET/WS/icinga2/prefix/var/lib/icinga2/icinga2.state'
[2023-05-11 17:06:53 +0200] information/ConfigObject: Restored 1 objects. Loaded 0 new objects without state.
[2023-05-11 17:06:53 +0200] information/ConfigItem: Triggering Start signal for config items
[2023-05-11 17:06:53 +0200] information/ConfigItem: Activated all objects.
^C[2023-05-11 17:06:57 +0200] information/Application: Received request to shut down.
[2023-05-11 17:06:57 +0200] information/Application: Shutting down...
[2023-05-11 17:06:57 +0200] information/ConfigObject: Dumping program state to file '/Users/aklimov/NET/WS/icinga2/prefix/var/lib/icinga2/icinga2.state'
[2023-05-11 17:06:57 +0200] information/IcingaApplication: Icinga has shut down.
$

[Al2Klimov]

Also Activity Monitor confirms different thread amounts for -DConfiguration.Concurrency=1, nothing and -DConfiguration.Concurrency=42.

[julianbrost]

Tested it and should be fine apart from that small spelling detail below that doesn't have an effect on the functionality.