Page MenuHomePhabricator

Avoid locking mutexes that are already held by the same thread
ClosedPublic

Authored by fpelliccioni on Tue, Sep 24, 12:35.

Details

Summary

Avoid locking mutexes that are already held by the same thread. These are reentrant mutexes, but still no need to lock them more than once per thread :-)

Backport of Bitcoin Core PR11762
https://github.com/bitcoin/bitcoin/pull/11762

Test Plan
  1. Build with Clang in Debug mode:
CXX=clang++ CC=clang cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check
  1. Verify that the compiler has not emitted a thread-safety warning.
  2. Run the node: ./src/bitcoind -regtest
  3. Verify that text similar to "Assertion failed: lock ... not held ..." is not printed on stderr.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Tue, Sep 24, 12:35
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Sep 24, 12:35
deadalnix accepted this revision.Tue, Sep 24, 13:49
This revision is now accepted and ready to land.Tue, Sep 24, 13:49
deadalnix requested changes to this revision.Tue, Sep 24, 13:50

The test plan is not apropriate. You need to test with clang to make sure the thread anotation are repsected, (gcc doesn't support this) and run the test suite on a debug build to check that AssertLockHeld doesn't trigger.

This revision now requires changes to proceed.Tue, Sep 24, 13:50
fpelliccioni edited the test plan for this revision. (Show Details)Tue, Sep 24, 15:06
fpelliccioni edited the test plan for this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)Tue, Sep 24, 15:15
fpelliccioni updated this revision to Diff 13115.Tue, Sep 24, 15:16
fpelliccioni edited the test plan for this revision. (Show Details)

Test Plan fixed.

Fabien accepted this revision.Wed, Sep 25, 05:55
deadalnix accepted this revision.Thu, Sep 26, 10:20
This revision is now accepted and ready to land.Thu, Sep 26, 10:20
fpelliccioni updated this revision to Diff 13170.Fri, Sep 27, 13:46

Rebase from master branch.