Give threads a recognisable name to aid in debugging #1515

muggenhor commented at 3:26 PM on June 24, 2012: contributor

These thread names are visible in gdb when using 'info threads'. Additionally both 'top' and 'ps' show these names unless told to display the command-line instead of task name.

sipa commented at 3:48 PM on June 24, 2012: member

ACK

gavinandresen commented at 4:00 PM on June 24, 2012: contributor

Good idea.

Diapolo commented at 4:11 PM on June 24, 2012: none

Sounds good.

laanwj commented at 1:42 PM on June 25, 2012: member

Good idea.

I've only got a comment about the thread names: it makes sense to link them to thread names in in the enumeration in net.h (sockethandler, openconnections, messagehandler, miner, etc...) to be consistent and for easy debugging. Or add least add a comment in net.h by which name each of the threads can be recognized.

muggenhor commented at 5:25 AM on June 26, 2012: contributor

@laanwj I've tried to make the names look like those in the thread enumeration, unfortunately only 15 characters are available. So I've deviated from those names for some instances to improve readability. That being said I could add a copy of the thread names as comments to that enum.

in src/util.h:None in 39faf4e280 outdated

 588 | @@ -585,6 +589,16 @@ inline void ExitThread(size_t nExitCode)
 589 |  }
 590 |  #endif
 591 |  
 592 | +static inline void RenameThread(const char* name)
 593 | +{
 594 | +#if defined(__linux__) && defined(PR_SET_NAME)

luke-jr commented at 5:48 PM on June 28, 2012:

Is there a reason to check for linux specifically?

muggenhor commented at 2:53 PM on June 30, 2012:

That syscall with that parameter is Linux specific, additionally the PR_SET_NAME checking-part is because it's only available on Linux upwards of 2.6.9.

luke-jr commented at 3:11 PM on June 30, 2012:

Yes, my point was that just checking for PR_SET_NAME should probably suffice?

muggenhor commented at 6:09 PM on June 30, 2012:

Right, I suppose it does, removed the linux part.

in src/bitcoinrpc.cpp:None in 39faf4e280 outdated

1775 |  }
1776 |  
1777 |  void ThreadCleanWalletPassphrase(void* parg)
1778 |  {
1779 | +    // Make this thread recognisable as the wallet unlocking thread
1780 | +    RenameThread("bitcoin-unlo-wa");

luke-jr commented at 5:50 PM on June 28, 2012:

But doesn't this thread relock the wallet? So "bitcoin-lock-wlt"

muggenhor commented at 3:06 PM on June 30, 2012:

Right, fixed it.

in src/main.cpp:None in 39faf4e280 outdated

3563 | @@ -3564,6 +3564,9 @@ void static BitcoinMiner(CWallet *pwallet)
3564 |      printf("BitcoinMiner started\n");
3565 |      SetThreadPriority(THREAD_PRIORITY_LOWEST);
3566 |  
3567 | +    // Make this thread recognisable as the mining thread
3568 | +    RenameThread("bitcoin [miner]");

luke-jr commented at 5:51 PM on June 28, 2012:

"bitcoin-miner" for consistency... (some similar ones later)

muggenhor commented at 3:06 PM on June 30, 2012:

Fixed.

luke-jr commented at 5:55 PM on June 28, 2012: member

Might add pthread_set_name_np for BSD.

I haven't checked, but it would be annoying if this erases the commandline parameters - I use a dummy -name=foo to discern between different bitcoinds in ps right now.

muggenhor commented at 3:05 PM on June 30, 2012: contributor

@luke-jr If pthread_set_name_np is implemented similar to BSD's setprogname then it will modify the commandline parameters (that's how it does this: overwriting argv[0-n] memory).

It's manpage seems to suggest otherwise though. I can add the code easily enough, but I'm not even sure if my FreeBSD 8 installation still boots. So I'd appreciate if you could test & confirm that it works as advertised,

luke-jr commented at 3:12 PM on June 30, 2012: member

@muggenhor I only use Linux, just came across the BSD variant looking at your pullreq.

muggenhor commented at 3:25 PM on June 30, 2012: contributor

@luke-jr Right, I've just confirmed that I can cross-compile an isolated call to pthread_set_name_np for FreeBSD 8.

I'll try dusting off my FreeBSD 8 box for a test run.

muggenhor commented at 6:03 PM on June 30, 2012: contributor

Right, my FreeBSD system doesn't even boot properly anymore. So I'll disable the FreeBSD code (but leave it in) so that it won't break on those systems but another developer (who does have FreeBSD/OpenBSD) can confirm that it does work and enable it.

in src/util.h:None in 6475cd36e6 outdated

 588 | @@ -585,6 +589,21 @@ inline void ExitThread(size_t nExitCode)
 589 |  }
 590 |  #endif
 591 |  
 592 | +static inline void RenameThread(const char* name)

laanwj commented at 8:17 AM on July 8, 2012:

Please move this implementation (and associated includes such as sys/prctl.h) to the .cpp instead of the .h, there is no need for inlining here.

muggenhor commented at 6:22 PM on July 8, 2012:

ACK, see rebased commits.

in src/net.cpp:None in 041856e55d outdated

 406 | @@ -407,6 +407,9 @@ bool GetMyExternalIP(CNetAddr& ipRet)
 407 |  
 408 |  void ThreadGetMyExternalIP(void* parg)

Diapolo commented at 7:56 PM on July 8, 2012:

I'm just asking myself, why not all Thread use IMPLEMENT_RANDOMIZE_STACK? Even if this is unrelated to this pull, what's the reason for that?

muggenhor commented at 9:27 PM on July 8, 2012:

When put like that I'd like to ask why IMPLEMENT_RANDOMIZE_STACK isn't implemented in CreateThread instead... Would call for a lot less duplication of code that way.

In fact, why are we using CreateThread at all the way it is right now (as some kind of Windows API-like-wrapper) instead of Boost.Thread? Especially considering that the latter can be passed arbitrary parameters of arbitrary type upon creation.

Diapolo commented at 5:06 AM on July 9, 2012:

I like the first idea, saves a few lines of code and we won't forget to add it (perhaps as another pull or commit for this one?).

I can't comment on the Boost.Thread stuff, but as we include or somewhere else use it (the lib is there), why not ... perhaps main devs can comment here.

muggenhor commented at 11:46 PM on July 16, 2012:

@Diapolo adding IMPLEMENT_RANDOMIZE_STACK to CreateThread would probably be best for another pull request.

Diapolo commented at 7:57 PM on July 8, 2012: none

In qtipcserver.cpp we have another thread afaik, any reason to not cover that one in this pull?

muggenhor commented at 9:24 PM on July 8, 2012: contributor

@Diapolo: that was just an oversight. Addressed in my last commit.

Give threads a recognisable name to aid in debugging

NOTE: These thread names are visible in gdb when using 'info threads'.
      Additionally both 'top' and 'ps' show these names *unless* told to
      display the command-line instead of task name.

Signed-off-by: Giel van Schijndel <me@mortis.eu>

96931d6f78

Fix thread names after review

 * Fix wrong thread name for wallet *relocking* thread
  - Was named the unlocking thread
 * Use consistent naming

Signed-off-by: Giel van Schijndel <me@mortis.eu>

9f46ab62b1

Add support for renaming FreeBSD and OpenBSD threads

NOTE: This is currently disabled, until a developer with FreeBSD/OpenBSD
      can confirm that this works (without causing undefined behaviour
      preferrably).

Signed-off-by: Giel van Schijndel <me@mortis.eu>

304ca95508

Don't check for __linux__ specifically, check for PR_SET_NAME feature instead

Signed-off-by: Giel van Schijndel <me@mortis.eu>

b277b0f100

Give the GUI-IPC thread a name as well

Signed-off-by: Giel van Schijndel <me@mortis.eu>

36fe96581f

muggenhor commented at 11:54 PM on July 16, 2012: contributor

Rebased again, still waiting to be merged...

gavinandresen merged this on Jul 17, 2012

gavinandresen closed this on Jul 17, 2012

suprnurd referenced this in commit 9268a336dd on Dec 5, 2017

lateminer referenced this in commit 5664663adb on Jan 22, 2019

lateminer referenced this in commit 24bc866346 on May 6, 2020

DrahtBot locked this on Sep 8, 2021