proxy: Name threads spawned by the event loop - #324
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| pthread_setname_np(name); | ||
| #else | ||
| // Prevent warnings for unused parameters... | ||
| (void)name; |
There was a problem hiding this comment.
In commit "proxy: Name threads spawned by the event loop" (61b6cd2)
Note for followup probably will want to extend this to windows.
There was a problem hiding this comment.
I agree, for Bitcoin Core it would be useful too.
There was a problem hiding this comment.
Opened for bitcoin core: bitcoin/bitcoin#35884
61b6cd2 to
67af015
Compare
|
Thanks for the review @ryanofsky! I forced push 67af015 so now it uses the macro HAVE_PTHREAD_GETTHREADID_NP instead. |
There was a problem hiding this comment.
This looks good, and i can think of usecases for development and debugging,
But i wonder why no test covergae was added for this change? is there a reason for this? I think a simple test to test the behaviour of the method would be nice to have. Perhaps something like this?
KJ_TEST("IPC worker has OS thread name")
{
TestSetup setup;
ProxyClient<messages::FooInterface>* foo = setup.client.get();
foo->initThreadMap();
setup.server->m_impl->m_fn = [] {};
EventLoop& loop = *setup.server->m_context.connection->m_loop;
std::promise<std::string> thread_name;
auto thread_name_future = thread_name.get_future();
loop.testing_hook_makethread_created = [&] {
thread_name.set_value(ThreadName(""));
};
foo->callFnAsync();
const std::string name = thread_name_future.get();
KJ_EXPECT(name.find("/capnp-worker-") != std::string::npos,
name);
}
Thanks for reviewing! Good point. tbh I thought it wound't be worth it but since this changes visible behavior (logs append OS thread name) it does make sense to add tests. Pushed 56cac0d: it adds tests under macro HAVE_PTHREAD_GETNAME_NP, so it only tests platforms that can get the thread OS name. |
|
Just thinking out loud and out of scope here: it might be worth storing the thread name in a |
4 participants
Threads spawned by makeThread(), makePool() and the async cleanup thread inherit the name of the thread that created them, so tooling and log lines can't distinguish them from it. This PR rename them at creation.
bitcoin-node
bitcoin-wallet
Since ThreadName() reads the name back with pthread_getname_np(), log lines pick this up too: