Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use shared lock for wallet registration functions. #2828

Closed

Conversation

CodeShark
Copy link
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.

…tions that modify the setpwalletRegistered container structure itself (and not the contents) need exclusive access.
@sipa
Copy link
Member

sipa commented Jul 14, 2013

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
Copy link
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
Copy link

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:

  1. 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)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. 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
Copy link
Contributor

jgarzik commented Jul 22, 2013

Agree, we cannot merge this and leave pull testing broken :/

@Diapolo
Copy link

Diapolo commented Jul 22, 2013

Perhaps another Boost bug related to an old version we use?

@CodeShark
Copy link
Contributor Author

Well, I'm glad at least this pull request did something interesting :p

@jgarzik
Copy link
Contributor

jgarzik commented Aug 25, 2013

Closing... re-open if/when issues are fixed.

@jgarzik jgarzik closed this Aug 25, 2013
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 8, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants