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

Fix CCheckQueue IsIdle (potential) race condition #9495

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@JeremyRubin
Contributor

JeremyRubin commented Jan 9, 2017

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 CCheckqueueControl is made, most likely waiting until control is relinquished is the right behavior rather than failing an assert.

@@ -185,8 +182,7 @@ class CCheckQueueControl
{
// passed queue is supposed to be unused, or NULL
if (pqueue != NULL) {
bool isIdle = pqueue->IsIdle();
assert(isIdle);
ENTER_CRITICAL_SECTION(pqueue->ControlMutex);

This comment has been minimized.

@sipa

sipa Jan 9, 2017

Member

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)?

@sipa

sipa Jan 9, 2017

Member

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)?

This comment has been minimized.

@JeremyRubin

JeremyRubin Jan 9, 2017

Contributor

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.

@JeremyRubin

JeremyRubin Jan 9, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Jan 9, 2017

Member

Understood. Let's improve that in a separate PR later.

@sipa

sipa Jan 9, 2017

Member

Understood. Let's improve that in a separate PR later.

@JeremyRubin JeremyRubin referenced this pull request Jan 9, 2017

Merged

CCheckQueue Unit Tests #9497

@ryanofsky

utACK 958ee1d 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

utACK 2a58f73

Show outdated Hide outdated src/checkqueue.h
if (pqueue != NULL) {
ENTER_CRITICAL_SECTION(pqueue->ControlMutex);
}
CCheckQueueControl<T>() = delete;

This comment has been minimized.

@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Nit: I think you can drop the <T>

@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Nit: I think you can drop the <T>

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Jan 20, 2017

Contributor

Squashed and addressed @ryanofsky's nit about template args in constructors.

Contributor

JeremyRubin commented Jan 20, 2017

Squashed and addressed @ryanofsky's nit about template args in constructors.

Show outdated Hide outdated src/checkqueue.h
CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
explicit CCheckQueueControl(CCheckQueue<T> * const pqueueIn) : pqueue(pqueueIn), fDone(false)
{
// passed queue is supposed to be unused, or NULL

This comment has been minimized.

@sipa

sipa Jan 20, 2017

Member

Weird indendation.

@sipa

sipa Jan 20, 2017

Member

Weird indendation.

This comment has been minimized.

@JeremyRubin

JeremyRubin Jan 20, 2017

Contributor

Oops -- somehow, my expandtab and tabstop=4 got unset.

@JeremyRubin

JeremyRubin Jan 20, 2017

Contributor

Oops -- somehow, my expandtab and tabstop=4 got unset.

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Jan 20, 2017

Contributor

sorry for causing extra review -- pushed and squashed indention fix.

Contributor

JeremyRubin commented Jan 20, 2017

sorry for causing extra review -- pushed and squashed indention fix.

@@ -127,6 +127,9 @@ class CCheckQueue
}
public:
//! Mutex to ensure only one concurrent CCheckQueueControl
boost::mutex ControlMutex;

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

Nit: Is there some coding standard for mutexes which makes you capitalize this ivar? If not, maybe controlMutex would be more consistent.

@kallewoof

kallewoof Feb 28, 2017

Member

Nit: Is there some coding standard for mutexes which makes you capitalize this ivar? If not, maybe controlMutex would be more consistent.

@JeremyRubin JeremyRubin referenced this pull request Mar 7, 2017

Closed

Lock-Free CheckQueue #9938

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Mar 27, 2017

Contributor

#9497 was merged, which included this. Closing.

Contributor

JeremyRubin commented Mar 27, 2017

#9497 was merged, which included this. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment