Skip to content

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Oct 3, 2019

Set only the internal name for the main threads.

Fixes #17036 for both bitcoind and bitcoin-qt.

After this, e.g. killall works again for either.

@laanwj laanwj added this to the 0.19.0 milestone Oct 3, 2019
@laanwj laanwj force-pushed the 2019_10_no_main_thread_rename branch from 5a2736e to 98b6d19 Compare October 3, 2019 04:11
Set only the internal name.

Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.
@laanwj laanwj force-pushed the 2019_10_no_main_thread_rename branch from 0979347 to 07e4bdb Compare October 3, 2019 04:59
@laanwj
Copy link
Member Author

laanwj commented Oct 3, 2019

Ugh, I suppose this was added to set the internal name for logging. Pushed an alternative approach.

@laanwj laanwj changed the title Don't call ThreadRename for the main thread Don't rename main thread at process level Oct 3, 2019
@hebasto
Copy link
Member

hebasto commented Oct 3, 2019

Concept ACK

@promag
Copy link
Contributor

promag commented Oct 3, 2019

Tested ACK 07e4bdb, killall bitcoind and killall bitcoin-qt now just works!

@jonatack
Copy link
Member

jonatack commented Oct 3, 2019

Thanks for this; I tripped on this recently and wondered what was the issue. Testing.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 07e4bdb killall bitcoind shuts down bitcoind mainnet/testnet/regtest, killall bitcoin-qt shuts down ./src/qt/bitcoin-qt, tests pass, very light code review. Good idea to add the @note warning. Thanks!

laanwj added a commit that referenced this pull request Oct 3, 2019
07e4bdb Don't rename main thread at process level (Wladimir J. van der Laan)

Pull request description:

  Set only the internal name for the main threads.

  Fixes #17036 for both `bitcoind` and `bitcoin-qt`.

  After this, e.g. `killall` works again for either.

ACKs for top commit:
  promag:
    Tested ACK 07e4bdb, `killall bitcoind` and `killall bitcoin-qt` now just works!
  jonatack:
    ACK 07e4bdb `killall bitcoind` shuts down bitcoind mainnet/testnet/regtest, `killall bitcoin-qt` shuts down `./src/qt/bitcoin-qt`, tests pass, very light code review. Good idea to add the `@note` warning. Thanks!

Tree-SHA512: 8f310ae646c83a02de7cc6869aa9aca1d53613d8fb762d05e3dfa52e17ca82abeb99044564cf7ba45b3c4b320e65bf8315d0e8834a9e696f097be5af638c6fd9
@laanwj laanwj merged commit 07e4bdb into bitcoin:master Oct 3, 2019
laanwj added a commit that referenced this pull request Oct 3, 2019
Set only the internal name.

Fixes #17036 for both `bitcoind` and `bitcoin-qt`.

Github-Pull: #17038
Rebased-From: 07e4bdb
Tree-SHA512: ed6f1b95a23c4c7863982ee6972429be5af0702ea93f0f17d32d2ef4b01446b1c0528eeadc45289609eda5c02ea68b3d722b8ecdfdf4fff4b02592c2188cc0a0
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 07e4bdb, I have tested the code on Linux Mint 19.2: killall {bitcoind|bitcoin-qt} works as expected (even with bash completion).

Also, ps outputs for bitcoin-qt (as example) with this PR:

$ ps
  PID TTY          TIME CMD
 3137 pts/0    00:00:00 bash
17072 pts/0    00:00:07 bitcoin-qt
17100 pts/0    00:00:00 ps
$ ps -T
  PID  SPID TTY          TIME CMD
 3137  3137 pts/0    00:00:00 bash
17072 17072 pts/0    00:00:00 bitcoin-qt
17072 17073 pts/0    00:00:00 QXcbEventReader
17072 17074 pts/0    00:00:00 bitcoin:disk$0
17072 17075 pts/0    00:00:00 QDBusConnection
17072 17076 pts/0    00:00:00 gmain
17072 17077 pts/0    00:00:00 gdbus
17072 17078 pts/0    00:00:07 b-qt-init
17072 17079 pts/0    00:00:00 b-scriptch.0
17072 17080 pts/0    00:00:00 b-scriptch.1
17072 17081 pts/0    00:00:00 b-scriptch.2
17072 17082 pts/0    00:00:00 b-scheduler
17072 17083 pts/0    00:00:00 b-http
17072 17084 pts/0    00:00:00 b-httpworker.0
17072 17085 pts/0    00:00:00 b-httpworker.1
17072 17086 pts/0    00:00:00 b-httpworker.2
17072 17087 pts/0    00:00:00 b-httpworker.3
17072 17088 pts/0    00:00:00 b-qt-init
17072 17090 pts/0    00:00:00 b-torcontrol
17072 17091 pts/0    00:00:00 b-upnp
17072 17092 pts/0    00:00:00 b-net
17072 17093 pts/0    00:00:00 b-dnsseed
17072 17094 pts/0    00:00:00 b-addcon
17072 17095 pts/0    00:00:00 b-opencon
17072 17096 pts/0    00:00:00 b-msghand
17072 17097 pts/0    00:00:00 QThread
17072 17098 pts/0    00:00:00 QThread
17105 17105 pts/0    00:00:00 ps

@maflcko
Copy link
Member

maflcko commented Oct 3, 2019

ACK

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 4, 2019
07e4bdb Don't rename main thread at process level (Wladimir J. van der Laan)

Pull request description:

  Set only the internal name for the main threads.

  Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

  After this, e.g. `killall` works again for either.

ACKs for top commit:
  promag:
    Tested ACK 07e4bdb, `killall bitcoind` and `killall bitcoin-qt` now just works!
  jonatack:
    ACK 07e4bdb `killall bitcoind` shuts down bitcoind mainnet/testnet/regtest, `killall bitcoin-qt` shuts down `./src/qt/bitcoin-qt`, tests pass, very light code review. Good idea to add the `@note` warning. Thanks!

Tree-SHA512: 8f310ae646c83a02de7cc6869aa9aca1d53613d8fb762d05e3dfa52e17ca82abeb99044564cf7ba45b3c4b320e65bf8315d0e8834a9e696f097be5af638c6fd9
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 17, 2019
Set only the internal name.

Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

Github-Pull: bitcoin#17038
Rebased-From: 07e4bdb
Tree-SHA512: ed6f1b95a23c4c7863982ee6972429be5af0702ea93f0f17d32d2ef4b01446b1c0528eeadc45289609eda5c02ea68b3d722b8ecdfdf4fff4b02592c2188cc0a0
fxtc pushed a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
Set only the internal name.

Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

Github-Pull: bitcoin#17038
Rebased-From: 07e4bdb
Tree-SHA512: ed6f1b95a23c4c7863982ee6972429be5af0702ea93f0f17d32d2ef4b01446b1c0528eeadc45289609eda5c02ea68b3d722b8ecdfdf4fff4b02592c2188cc0a0
fxtc pushed a commit to fxtc/fxtc that referenced this pull request Nov 25, 2019
Set only the internal name.

Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

Github-Pull: bitcoin#17038
Rebased-From: 07e4bdb
Tree-SHA512: ed6f1b95a23c4c7863982ee6972429be5af0702ea93f0f17d32d2ef4b01446b1c0528eeadc45289609eda5c02ea68b3d722b8ecdfdf4fff4b02592c2188cc0a0
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 24, 2020
Summary:
```
Set only the internal name.

Fixes #17036 for both `bitcoind` and `bitcoin-qt`.
```

This fixes Unix tools like `ps` or `pkill` that use the process name
(called `bitcoin-init` since D5540).

Backport of core [[bitcoin/bitcoin#17038 | PR17038]].

Depends on D5540.

Test Plan:
  ninja check

  bitcoind -daemon
  ps -A | grep bitcoind # should return our bitcoind instance
  sudo killall bitcoind
  ps -A | grep bitcoind # should return nothing, kill successful

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D5544
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Dec 1, 2020
Thread names in logs and deadlock debug tools

See merge request bitcoin-cash-node/bitcoin-cash-node!834

This is a backport of [PR15849](bitcoin/bitcoin#15849) (Thread names in logs and deadlock debug tools).

See original PR for benchmarks.

Also includes some smaller follow-up PR's:
* [PR15968](bitcoin/bitcoin#15968) - Fix portability issue with pthreads
* [PR16984](bitcoin/bitcoin#16984) - util: Make thread names shorter
* [PR17038](bitcoin/bitcoin#17038) - Don't rename main thread at process level

## Test plan

New tests: `ninja check`

Run with `-logthreadnames`, observe lines are prefixed with thread

```
2020-10-28T11:39:16Z [init] init message: Done loading
2020-10-28T11:39:16Z [opencon] opencon thread start
2020-10-28T11:39:16Z [dnsseed] 0 addresses found from DNS seeds
2020-10-28T11:39:16Z [msghand] msghand thread start
2020-10-28T11:39:16Z [dnsseed] dnsseed thread exit
```

Compile with lockorder debugging enabled: `cmake -GNinja -DCMAKE_BUILD_TYPE=Debug ..`
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 21, 2021
07e4bdb Don't rename main thread at process level (Wladimir J. van der Laan)

Pull request description:

  Set only the internal name for the main threads.

  Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

  After this, e.g. `killall` works again for either.

ACKs for top commit:
  promag:
    Tested ACK 07e4bdb, `killall bitcoind` and `killall bitcoin-qt` now just works!
  jonatack:
    ACK 07e4bdb `killall bitcoind` shuts down bitcoind mainnet/testnet/regtest, `killall bitcoin-qt` shuts down `./src/qt/bitcoin-qt`, tests pass, very light code review. Good idea to add the `@note` warning. Thanks!

Tree-SHA512: 8f310ae646c83a02de7cc6869aa9aca1d53613d8fb762d05e3dfa52e17ca82abeb99044564cf7ba45b3c4b320e65bf8315d0e8834a9e696f097be5af638c6fd9
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 12, 2021
07e4bdb Don't rename main thread at process level (Wladimir J. van der Laan)

Pull request description:

  Set only the internal name for the main threads.

  Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

  After this, e.g. `killall` works again for either.

ACKs for top commit:
  promag:
    Tested ACK 07e4bdb, `killall bitcoind` and `killall bitcoin-qt` now just works!
  jonatack:
    ACK 07e4bdb `killall bitcoind` shuts down bitcoind mainnet/testnet/regtest, `killall bitcoin-qt` shuts down `./src/qt/bitcoin-qt`, tests pass, very light code review. Good idea to add the `@note` warning. Thanks!

Tree-SHA512: 8f310ae646c83a02de7cc6869aa9aca1d53613d8fb762d05e3dfa52e17ca82abeb99044564cf7ba45b3c4b320e65bf8315d0e8834a9e696f097be5af638c6fd9
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 6, 2021
Set only the internal name.

Fixes bitcoin#17036 for both `bitcoind` and `bitcoin-qt`.

Github-Pull: bitcoin#17038
Rebased-From: 07e4bdb
Tree-SHA512: ed6f1b95a23c4c7863982ee6972429be5af0702ea93f0f17d32d2ef4b01446b1c0528eeadc45289609eda5c02ea68b3d722b8ecdfdf4fff4b02592c2188cc0a0
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming main thread causes issues with UNIX utilities
5 participants