Prevent mutex lock fail even if --enable-debug

AkioNak commented at 3:03 am on January 23, 2019: contributor

This PR intends to resolve #15227.

configure --enable-debug enables #ifdef DEBUG_LOCKORDER.

Then lockdata (in sync.cpp) will be initialized same as other static objects.

But unfortunately, lockdata.push_lock() was called before its initialization (via initializing signatureCache which is declared in script/sigcache.cpp) on macOS.

This PR apply the “Construct On First Use Idiom” to lockdata to prevent it.

edited — fix typo.

in src/sync.cpp:77 in 6a5e3c2dd2 outdated

72@@ -73,7 +73,11 @@ struct LockData {
73     LockOrders lockorders;
74     InvLockOrders invlockorders;
75     std::mutex dd_mutex;
76-} static lockdata;
77+};
78+LockData& get_lockdata() {

sipa commented at 3:15 am on January 23, 2019:

Nit: function naming style (GetLockData ?).

in src/sync.cpp:78 in 6a5e3c2dd2 outdated

72@@ -73,7 +73,11 @@ struct LockData {
73     LockOrders lockorders;
74     InvLockOrders invlockorders;
75     std::mutex dd_mutex;
76-} static lockdata;
77+};
78+LockData& get_lockdata() {
79+    static LockData* lockdata = new LockData();

sipa commented at 3:16 am on January 23, 2019:

Can you use a std::unique_ptr<LockData> here instead? This code will technically cause a memory leak (as the LockData won’t get cleaned up at shutdown).

kallewoof commented at 3:44 am on January 23, 2019:

Hm? Why would it cause a leak? All memory used by a process is always released upon ~shutdown~ termination.

sipa commented at 4:07 am on January 23, 2019:

@kallewoof Yes, that’s why I say technically. We generally ain to follow that rule though, as it means leak detection systems like valgrind don’t report spurious leaks.

kallewoof commented at 4:10 am on January 23, 2019:

I thought valgrind was intelligent about static vars inside functions, and always considered this type of expression to be acceptable. I guess a unique pointer is less dirty though, so concept ACK on the change.

sipa commented at 4:15 am on January 23, 2019:

@kallewoof Good point, I hadn’t considered that actually. Some googling indicates that in cases like this, the memory would indeed be counted as still reachable rather than lost.

Still, using a unique_ptr here comes at no cost, and is more obviously correct.

AkioNak commented at 5:25 am on January 23, 2019:

@sipa @kallewoof Thank you for the discussion. I will address it.

practicalswift commented at 7:56 am on January 23, 2019:

The consensus must be that we should aim for zero bytes “still reachable” when exiting, right? :-)

promag commented at 4:12 pm on January 24, 2019:

Is this thread safe?

sipa commented at 4:17 pm on January 24, 2019:

Yes, since C++11 the language guarantees that the static initializer is only run once. If it is invoked from multiple threads simultaneously, only the first will complete, and the others will wait for the first to run and use the same result.

promag commented at 4:42 pm on January 24, 2019:

Could just be static LockData lockdata; right?

sipa commented at 5:30 pm on January 24, 2019:

@promag Oh, even better!

sipa commented at 3:17 am on January 23, 2019: member

Concept ACK

fanquake added the label Refactoring on Jan 23, 2019

kallewoof commented at 3:56 am on January 23, 2019: member

Tested ACK

Master branch make check results in failures (see below); with this PR, make check exits successfully.

0Making check in src
1/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS check-local
2../build-aux/test-driver: line 107: 29548 Abort trap: 6           "$@" > $log_file 2>&1

laanwj added this to the milestone 0.18.0 on Jan 24, 2019

Sjors commented at 4:21 pm on January 24, 2019: member

Tested 6a5e3c2 on macOS without GUI, as follows:

Toggling --enable-debug produced some compiler headaches for me, so I cleaned up in between.

Testing with --enable-debug:

0# on master
1make distclean
2./autogen.sh
3./configure --disable-bench --with-miniupnpc=no --with-incompatible-bdb --with-zmq --without-gui --enable-debug
4make && make check
5# test fails
6
7# switch to this branch
8make && make check
9# tests pass

Testing without --enable-debug, tests pass on master and with this change.

I also ran the benchmarks with --enable-debug, though only on this commit.

I’m still confused why this bug only affected macOS.

promag changes_requested

promag commented at 5:12 pm on January 24, 2019: member

Tested ACK, but I think you could make it:

0// Construct On First Use Idiom ...
1LockData& GetLockData()
2{
3    static LockData lockdata;
4    return lockdata;
5}

sipa commented at 3:48 am on January 25, 2019: member

utACK 416bf1b74aecfda51fa9d41b57e39fd0d488e742. Squash?

Prevent mutex lock fail even if --enable-debug

This PR intends to resolve #15227.

"configure --debug-enabled" enables "#ifdef DEBUG_LOCKORDER".
Then "lockdata" (in sync.cpp) will be initialized same as other
static objects.

But unfortunately, lockdata.push_lock() was called before its
initialization (via initializing signatureCache which is declared
in script/sigcache.cpp) on macOS.

This PR apply the "Construct On First Use Idiom" to "lockdata"
to prevent it.

b09dab0f2d

AkioNak force-pushed on Jan 25, 2019

AkioNak commented at 4:28 am on January 25, 2019: contributor

@promag Thanks for the suggestion. Fixed and squashed.

kallewoof commented at 4:43 am on January 25, 2019: member

tACK b09dab0

(It somehow escaped me that you could just do a static instance like that without making it a pointer. Mind blown. :)

MarcoFalke merged this on Jan 25, 2019

MarcoFalke closed this on Jan 25, 2019

MarcoFalke referenced this in commit d14ef5721f on Jan 25, 2019

in src/sync.cpp:77 in b09dab0f2d

72@@ -73,7 +73,11 @@ struct LockData {
73     LockOrders lockorders;
74     InvLockOrders invlockorders;
75     std::mutex dd_mutex;
76-} static lockdata;
77+};
78+LockData& GetLockData() {

promag commented at 8:23 am on January 25, 2019:

Err could also be static..

AkioNak commented at 10:54 am on January 25, 2019:

@promag Thanks for your reviewing. Does this mean that the scope of GetLockData() should be compile-unit local same as original lockdata to prevent external calling?

promag commented at 11:01 am on January 25, 2019:

Yes linkage changed, but I don’t think it warrants a PR.

AkioNak deleted the branch on Mar 13, 2019

zkbot referenced this in commit 05c0c7c573 on Apr 20, 2021

PastaPastaPasta referenced this in commit f0563e77bd on Jun 26, 2021

PastaPastaPasta referenced this in commit 5a788fa023 on Jun 26, 2021

PastaPastaPasta referenced this in commit dad75795a8 on Jun 27, 2021

PastaPastaPasta referenced this in commit a64061f518 on Jun 28, 2021

DrahtBot locked this on Dec 16, 2021

Prevent mutex lock fail even if –enable-debug #15233