Fix CCheckQueue IsIdle (potential) race condition #9495
Conversation
src/checkqueue.h
Outdated
There was a problem hiding this comment.
Can you do this as a unique_lock field inside CCheckQueueControl (so we don't need to rely on an explicit destructor to be correct)?
There was a problem hiding this comment.
Unfortunately this is the "easiest" way to take care of this given that we want debug order & the CMutex class has no default constructor/moveable semantics. Certainly can be fixed to RAII easily later.
There was a problem hiding this comment.
Understood. Let's improve that in a separate PR later.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 958ee1d3d844d1f2f07b271ad7ed9bb117e5a766 with one nit:
Would suggest deleting the CCheckQueueControl default copy constructor and copy operator and making the pqueue member const, to give more assurance that the ENTER/LEAVE calls will always be paired.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 2a58f73601ad36d0531fe9e69e1affd9229d8b96
src/checkqueue.h
Outdated
There was a problem hiding this comment.
Nit: I think you can drop the <T>
JeremyRubin
force-pushed
the
checkqueue-control-lock
branch
from
2a58f73 to
020acd8
Compare
|
Squashed and addressed @ryanofsky's nit about template args in constructors. |
src/checkqueue.h
Outdated
There was a problem hiding this comment.
Oops -- somehow, my expandtab and tabstop=4 got unset.
JeremyRubin
force-pushed
the
checkqueue-control-lock
branch
from
020acd8 to
f5daff7
Compare
|
sorry for causing extra review -- pushed and squashed indention fix. |
|
|
||
| public: | ||
| //! Mutex to ensure only one concurrent CCheckQueueControl | ||
| boost::mutex ControlMutex; |
There was a problem hiding this comment.
Nit: Is there some coding standard for mutexes which makes you capitalize this ivar? If not, maybe controlMutex would be more consistent.
|
#9497 was merged, which included this. Closing. |
4 participants
There's a small race condition in the CCheckQueue code which I don't think is currently an active issue, but future code might break.
IsIdle is not threadsafe. If two concurrent
CCheckqueueControls are made, they could simultaneously report being idle, and fail to panic.Furthermore, in the case a concurrent
CCheckqueueControlis made, most likely waiting until control is relinquished is the right behavior rather than failing an assert.