Using a shared lock for wallet registration functions since only functions that modify the setpwalletRegistered container structure itself (and not the contents) need exclusive access.
Use shared lock for wallet registration functions. #2828
pull CodeShark wants to merge 1 commits into bitcoin:master from CodeShark:wallet_registration_shared_lock changing 2 files +15 −19-
CodeShark commented at 5:08 PM on July 13, 2013: contributor
-
Using a shared lock for wallet registration functions since only functions that modify the setpwalletRegistered container structure itself (and not the contents) need exclusive access. 36f03801dc
-
sipa commented at 11:18 AM on July 14, 2013: member
Two comments:
- Shared locks are not recursive, so if any of these wallet callbacks causes a call to main, which causes a new callback, you have a deadlock. I don't believe such cases occur, but it's worth making a comment about it in the code.
- There is still a potential deadlock when callbacks are combined with modifications to setpwalletRegistered. This is not a problem for now, as such modifications only happen at startup, but we need to think about this before fully-fledged multiwallet is added. In general, it will probably mean that RPC calls cannot gratuitously lock the wallet anymore - only those that need access to a wallet, and only the one the need. In particular, anything modifying the set of registered wallets will need a lock on setpwalletRegistered without locking any of the wallets in it.
ACK though.
-
gavinandresen commented at 11:53 PM on July 19, 2013: contributor
This pull seems to reliably cause the pull-tester bot to hang, I don't know why (maybe deadlock is being triggered by these changes, or maybe there's a bug in the pull-tester script).
-
BitcoinPullTester commented at 4:25 AM on July 20, 2013: none
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/36f03801dc66193bdbdee25a92b871202dbd36c6 for binaries and test log.
This could happen for one of several reasons:
- It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
- It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
- It does not build on either Linux i386 or Win32 (via MinGW cross compile)
- The test suite fails on either Linux i386 or Win32
- The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
-
jgarzik commented at 3:22 PM on July 22, 2013: contributor
Agree, we cannot merge this and leave pull testing broken :/
-
Diapolo commented at 5:48 PM on July 22, 2013: none
Perhaps another Boost bug related to an old version we use?
-
CodeShark commented at 8:57 AM on July 26, 2013: contributor
Well, I'm glad at least this pull request did something interesting :p
-
jgarzik commented at 2:54 AM on August 25, 2013: contributor
Closing... re-open if/when issues are fixed.
- jgarzik closed this on Aug 25, 2013
- Bushstar referenced this in commit 3db4e3111d on Apr 8, 2020
- DrahtBot locked this on Sep 8, 2021
