ThreadPool: Add get_thread_index()

[alt-graph]

[why]

There are use cases in which it is beneficial to know on which of the N pool threads a function gets executed. Sascha recently came up with one such case for the new clientlib NameService.

[how]

Store a thread-local thread index when launching the threads. Return this index when called from a worker thread. For other threads, the thread-local index gets initialized to the maximum value of size_t; if this value is found, get_thread_index() throws an exception.

[alt-graph]

BTW: Did you notice this is MR 100? 🍾 🎆 🥳 🎉

[soerengrunewald]

Aside from the minor comment, it looks ok to me, so I will give it a 👍

[Finii]

Hmm, what can I, as a user, do with the get_thread_index() value?

The index exposes internals that we probably do not want to expose (apart from other things).

Maybe there is another way to solve Sascha's problem; would be good to know why he thinks he needs the index of the thread in the vector. Which for example assumes there is a order of the threads in the pool, or that the pool is held in some array kind structure etc pp.

[Finii]

This is a new concept, there is no order and no indexes before this change.
It is rather an implementation detail.

Maybe we want to return a get_thread_Id() instead? Analogous to TaskId. That would leak nothing (even if technically the same value).

But without knowledge of the problem - which has not been described in the PR description - we can not judge what this is all about.

I.e.: Not approve (yet)

[alt-graph]

Hmm, what can I, as a user, do with the get_thread_index() value?

The index exposes internals that we probably do not want to expose (apart from other things).

Maybe there is another way to solve Sascha's problem; would be good to know why he thinks he needs the index of the thread in the vector. Which for example assumes there is a order of the threads in the pool, or that the pool is held in some array kind structure etc pp.

See https://mcs-gitlab.desy.de/doocs/doocs-core-libraries/clientlib/-/merge_requests/321 for Sascha's merge request. Yes, I guess one could argue that we expose implementation details with get_thread_index()... 🤔

[Finii]

And IF this is about identifying the threads (see if it is the same or something), why not use threads::get_id().

I'm am completely in the dark. What would one need the ... (implementation details that I cant access from the outside anyhow) ... as a user?

Edit: Typo ID -> IF

[alt-graph]

And IF this is about identifying the threads (see if it is the same or something), why not use threads::get_id().

This is the solution used now by Sascha, but it would be much less complicated if one could just ask the pool about a thread index.

I'm am completely in the dark. What would one need the ... (implementation details that I cant access from the outside anyhow) ... as a user?

In a nutshell, it is about using resources that are bound to one specific thread. Thread #0 should access resource #0, thread #1 should access resource #1, and so on. And injecting the data into the thread via TLS or so does not seem to be an option.

I guess one of the questions is: Does adding get_thread_index() stop us from enhancing the API in some way in the future?

[Finii]

This is the solution used now by Sascha, but it would be much less complicated if one could just ask the pool about a thread index.

I can not really see get_id in the mentioned PR's diff.
And I guess auto my_id = std::this_thread::get_id() is not very complicated?

I guess one of the questions is: Does adding get_thread_index() stop us from enhancing the API in some way in the future?

Why not call it get_thread_Id()? Still it seems that the number is used as ID and not as index, and so we do not need to have it assume there is an index and not it's just a unsorted pot full of threads.

But if we call it get_thread_Id() we can also return THE thread id (thread::get_id) from that function; and then the user could do that herself instead?

Maybe you can point me to the use case in the PR, it is rather big and has numerous discussions ;-)

[alt-graph]

Maybe you can point me to the use case in the PR, it is rather big and has numerous discussions ;-)

To my understanding, NameService::get_thread_index() does more or less what is proposed here, just in a more complicated fashion. This index is then used for accessing entries of the ldap_ and hangup_task_ vectors. ldap_, specifically, holds OpenLDAP connections that are explicitly thread-unsafe, so each of them must be associated with one of our threads.

[Finii]

Hmm, as far as I can see the NameService::get_thread_index() is used to access the thread-associated data in ldap_ for example.

But there is no guarantee that the thread pool worker with thread-pool::get_thread_index() has the same index in the ldap_ etc vectors?

And interestingly the hangup task gets the index as captured value

    hangup_task_[ldap_indx]                                               
        = pool_->add_task([this, ldap_indx]() { hang_up(ldap_indx); }, hangup_duration_);

Ah now I get it, the resources held in ldap_ are just used by some thread and it will always be the same number, it does not need to be the index in fact.

🤔

[Finii]

Hmm, at least I think we should/could call this ID, even if it is the index.

But then, when I look into NameService.* I wonder why this solution has been choosen. A lot pre-created LdapConnection objects stored as shared ptr in a ("global") vector. Not even a std::array 🤔 Ah this is to not-initialize the ldap connection "often".

Some other idea to solve this:

A ThreadPool could also handle resources that the workers usually need (in this case LdapConnection objects).
For me that would be the more natural change. Then NameService does not need to store the connections itself but offloads that to the thread pool; and the thread in the pool always has its own resource, guaranteed.

The change is a bit tricky because we generally do not know the type of the resource that each thread needs and it is hard(er) to specify. I mean, in C-isch land the ThreadPool could store unique-ptr to void ;) and the ThreadPool::ThreadPool() gets a factory function pointer or similar to produce that resource ptrs.

I can not see the solution clearly, but that would need some experiments, but I have the feeling that such an approach would also make the NameService stuff more clear. It looks a lot like he worked around the problem that thread-pool threads can not have precreated resources, and that seems to be not a very strange request. So the ThreadPool could maybe gain something from the problem and not just bare solve the issue.

🤔

[alt-graph]

A ThreadPool could also handle resources that the workers usually need (in this case LdapConnection objects).
For me that would be the more natural change. Then NameService does not need to store the connections itself but offloads that to the thread pool; and the thread in the pool always has its own resource, guaranteed.

Yes, that could be an idea, but that would clearly be another PR.

[alt-graph]

Sorry, I messed up the last commit. It is corrected now, and the checks pass.

[Finii]

[alt-graph]

Thanks a lot, @soerengrunewald and @Finii!