-
Notifications
You must be signed in to change notification settings - Fork 600
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
waddrmgr: fix deadlock #967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find, LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM👍
We found some other places where locking was broken, see: |
@aakselrod would you have time to create a PR from the two commits linked above? |
Thanks for the hint, @losh11. Though I took a quick look at the two commits and it looks like the way @aakselrod fixed the code in this PR (turning I merged the two commits and rebased/fixed them for |
This PR fixes a deadlock caused by methods of
ScopedManager
read-locking the root manager forWatchOnly()
andLocked()
calls while theScopedManager
itself is locked. This is the reverse order from the normal way of taking locks, but has been hidden by the fact that DB transactions have never happened in parallel until the upcoming postgres changes in LND.See discussion here for more details.
The first commit updates a test to cause this failure. The following commits, pushed after CI completes/tests fail, contain the fix.