Fix CCheckQueue IsIdle (potential) race condition and remove dangerous constructors.

JeremyRubin · JeremyRubin · commit e2073424fd5a · 2017-01-20T18:18:59.000-05:00
diff --git a/src/checkqueue.h b/src/checkqueue.h
@@ -127,6 +127,9 @@ class CCheckQueue
     }
 
 public:
+    //! Mutex to ensure only one concurrent CCheckQueueControl
+    boost::mutex ControlMutex;
+
     //! Create a new check queue
     CCheckQueue(unsigned int nBatchSizeIn) : nIdle(0), nTotal(0), fAllOk(true), nTodo(0), fQuit(false), nBatchSize(nBatchSizeIn) {}
 
@@ -161,12 +164,6 @@ class CCheckQueue
     {
     }
 
-    bool IsIdle()
-    {
-        boost::unique_lock<boost::mutex> lock(mutex);
-        return (nTotal == nIdle && nTodo == 0 && fAllOk == true);
-    }
-
 };
 
 /** 
@@ -177,16 +174,18 @@ template <typename T>
 class CCheckQueueControl
 {
 private:
-    CCheckQueue<T>* pqueue;
+    CCheckQueue<T> * const pqueue;
     bool fDone;
 
 public:
-    CCheckQueueControl(CCheckQueue<T>* pqueueIn) : pqueue(pqueueIn), fDone(false)
+    CCheckQueueControl() = delete;
+    CCheckQueueControl(const CCheckQueueControl&) = delete;
+    CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
+    explicit CCheckQueueControl(CCheckQueue<T> * const pqueueIn) : pqueue(pqueueIn), fDone(false)
     {
         // passed queue is supposed to be unused, or NULL
         if (pqueue != NULL) {
-            bool isIdle = pqueue->IsIdle();
-            assert(isIdle);
+            ENTER_CRITICAL_SECTION(pqueue->ControlMutex);
         }
     }
 
@@ -209,6 +208,9 @@ class CCheckQueueControl
     {
         if (!fDone)
             Wait();
+        if (pqueue != NULL) {
+            LEAVE_CRITICAL_SECTION(pqueue->ControlMutex);
+        }
     }
 };
 

Original file line number	Diff line number	Diff line change
`@@ -127,6 +127,9 @@ class CCheckQueue`
`127`	`127`	`}`
`128`	`128`
`129`	`129`	`public:`
	`130`	`+ //! Mutex to ensure only one concurrent CCheckQueueControl`
	`131`	`+ boost::mutex ControlMutex;`
	`132`	`+`
`130`	`133`	`//! Create a new check queue`
`131`	`134`	`CCheckQueue(unsigned int nBatchSizeIn) : nIdle(0), nTotal(0), fAllOk(true), nTodo(0), fQuit(false), nBatchSize(nBatchSizeIn) {}`
`132`	`135`
`@@ -161,12 +164,6 @@ class CCheckQueue`
`161`	`164`	`{`
`162`	`165`	`}`
`163`	`166`
`164`		`- bool IsIdle()`
`165`		`- {`
`166`		`- boost::unique_lock<boost::mutex> lock(mutex);`
`167`		`- return (nTotal == nIdle && nTodo == 0 && fAllOk == true);`
`168`		`- }`
`169`		`-`
`170`	`167`	`};`
`171`	`168`
`172`	`169`	`/**`
`@@ -177,16 +174,18 @@ template <typename T>`
`177`	`174`	`class CCheckQueueControl`
`178`	`175`	`{`
`179`	`176`	`private:`
`180`		`- CCheckQueue<T>* pqueue;`
	`177`	`+ CCheckQueue<T> * const pqueue;`
`181`	`178`	`bool fDone;`
`182`	`179`
`183`	`180`	`public:`
`184`		`- CCheckQueueControl(CCheckQueue<T>* pqueueIn) : pqueue(pqueueIn), fDone(false)`
	`181`	`+ CCheckQueueControl() = delete;`
	`182`	`+ CCheckQueueControl(const CCheckQueueControl&) = delete;`
	`183`	`+ CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;`
	`184`	`+ explicit CCheckQueueControl(CCheckQueue<T> * const pqueueIn) : pqueue(pqueueIn), fDone(false)`
`185`	`185`	`{`
`186`	`186`	`// passed queue is supposed to be unused, or NULL`
`187`	`187`	`if (pqueue != NULL) {`
`188`		`- bool isIdle = pqueue->IsIdle();`
`189`		`- assert(isIdle);`
	`188`	`+ ENTER_CRITICAL_SECTION(pqueue->ControlMutex);`
`190`	`189`	`}`
`191`	`190`	`}`
`192`	`191`
`@@ -209,6 +208,9 @@ class CCheckQueueControl`
`209`	`208`	`{`
`210`	`209`	`if (!fDone)`
`211`	`210`	`Wait();`
	`211`	`+ if (pqueue != NULL) {`
	`212`	`+ LEAVE_CRITICAL_SECTION(pqueue->ControlMutex);`
	`213`	`+ }`
`212`	`214`	`}`
`213`	`215`	`};`
`214`	`216`