validation: avoid potential deadlocks in ValidationInterface#15205
Closed
jamesob wants to merge 3 commits intobitcoin:masterfrom
Closed
validation: avoid potential deadlocks in ValidationInterface#15205jamesob wants to merge 3 commits intobitcoin:masterfrom
jamesob wants to merge 3 commits intobitcoin:masterfrom
Conversation
See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)
jamesob
force-pushed
the
2019-01-avoid-validationqueue-deadlock
branch
from
af05222 to
76d0fa3
Compare
ryanofsky
reviewed
Jan 18, 2019
jamesob
force-pushed
the
2019-01-avoid-validationqueue-deadlock
branch
2 times, most recently
from
37f3992 to
c024cc9
Compare
Contributor
|
Restarted appveyor, unrelated error. |
jamesob
commented
Jan 20, 2019
jamesob
force-pushed
the
2019-01-avoid-validationqueue-deadlock
branch
from
c024cc9 to
5a18208
Compare
Contributor
Author
|
Pushed an update futureproofing all calls to |
jamesob
force-pushed
the
2019-01-avoid-validationqueue-deadlock
branch
2 times, most recently
from
7c01deb to
6f330dc
Compare
Since ActivateBestChain will in certain cases block in order to allow the ValidationInterface queue to drain, it cannot safely be called from within ValidationInterface callbacks. This patch adds assertions preventing that when DEBUG_LOCKORDER. We slightly rework the ValidationInterface class, adding an `AddToProcessQueue()` method that handles scheduling a callback. When the scheduling queue is initiated, we make the thread aware it's managed by ValidationInterface.
Also adds default constructor and destructor to CMainSignals to avoid build errors due to the forward declaration of MainSignalsInstance.
jamesob
force-pushed
the
2019-01-avoid-validationqueue-deadlock
branch
from
6f330dc to
93e09ee
Compare
Contributor
Author
|
I've taken @practicalswift's advice on declaration conditional on |
Contributor
Author
|
We discussed a better approach today on IRC (http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-22.html), which is to conditionally move all |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, the potential for a deadlock exists if
ActivateBestChain()is called from within theValidationInterfacescheduler thread. This is because ABC itself avoids overrunning the VI queue by waiting for it to drain once it has passed a certain depth.To avoid this (or at least disallow it during CI runs), this change introduces thread_local state that gets set to indicate execution on ValidationInterface threads. If we've compiled with
DEBUG_LOCKORDER, calling ABC from within a VI thread throws an exception. A small,necessaryrefactoring onCMainSignalsis included.This changeset also includes a commit from #13168 that limits the platforms where we can rely on the use of thread_local.