Protect access to member with mutex in CBackgroundPersister (#18)

Author: davidkyle

include/api/CBackgroundPersister.h

@@ -23,9 +23,11 @@
 
 #include <api/ImportExport.h>
 
+#include <atomic>
 #include <functional>
 #include <list>
 
+class CBackgroundPersisterTest;
 
 namespace ml
 {
@@ -98,19 +100,11 @@ class API_EXPORT CBackgroundPersister : private core::CNonCopyable
         //! \return true if the function was added; false if not.
         bool addPersistFunc(core::CDataAdder::TPersistFunc persistFunc);
 
-        //! When this function is called a background persistence will be
-        //! triggered unless there is already one in progress.
-        bool startPersist(void);
-
-        //! Clear any persistence functions that have been added but not yet
-        //! invoked.  This will be rejected if a background persistence is
-        //! currently in progress.
-        //! \return true if the list of functions is clear; false if not.
-        bool clear(void);
-
         //! Set the first processor persist function, which is used to start the
         //! chain of background persistence.  This will be rejected if a
         //! background persistence is currently in progress.
+        //! This should be set once before startBackgroundPersistIfAppropriate is
+        //! called.
         bool firstProcessorPeriodicPersistFunc(const TFirstProcessorPeriodicPersistFunc &firstProcessorPeriodicPersistFunc);
 
         //! Check whether a background persist is appropriate now, and if it is
@@ -135,6 +129,17 @@ class API_EXPORT CBackgroundPersister : private core::CNonCopyable
                 CBackgroundPersister &m_Owner;
         };
 
+    private:
+        //! When this function is called a background persistence will be
+        //! triggered unless there is already one in progress.
+        bool startPersist(void);
+
+        //! Clear any persistence functions that have been added but not yet
+        //! invoked.  This will be rejected if a background persistence is
+        //! currently in progress.
+        //! \return true if the list of functions is clear; false if not.
+        bool clear(void);
+
     private:
         //! How frequently should background persistence be attempted?
         core_t::TTime                      m_PeriodicPersistInterval;
@@ -157,10 +162,10 @@ class API_EXPORT CBackgroundPersister : private core::CNonCopyable
         core::CFastMutex                   m_Mutex;
 
         //! Is the background thread currently busy persisting data?
-        volatile bool                      m_IsBusy;
+        std::atomic_bool                   m_IsBusy;
 
         //! Have we been told to shut down?
-        volatile bool                      m_IsShutdown;
+        std::atomic_bool                   m_IsShutdown;
 
         using TPersistFuncList = std::list<core::CDataAdder::TPersistFunc>;
 
@@ -173,6 +178,9 @@ class API_EXPORT CBackgroundPersister : private core::CNonCopyable
     // Allow the background thread to access the member variables of the owning
     // object
     friend class CBackgroundThread;
+
+    // For testing
+    friend class ::CBackgroundPersisterTest;
 };
 
 

lib/api/CBackgroundPersister.cc

@@ -100,7 +100,7 @@ bool CBackgroundPersister::addPersistFunc(core::CDataAdder::TPersistFunc persist
 
     core::CScopedFastLock lock(m_Mutex);
 
-    if (m_IsBusy)
+    if (this->isBusy())
     {
         return false;
     }
@@ -122,14 +122,14 @@ bool CBackgroundPersister::addPersistFunc(core::CDataAdder::TPersistFunc persist
 
 bool CBackgroundPersister::startPersist(void)
 {
-    if (m_PersistFuncs.empty())
+    core::CScopedFastLock lock(m_Mutex);
+
+    if (this->isBusy())
     {
         return false;
     }
 
-    core::CScopedFastLock lock(m_Mutex);
-
-    if (m_IsBusy)
+    if (m_PersistFuncs.empty())
     {
         return false;
     }
@@ -154,7 +154,7 @@ bool CBackgroundPersister::clear(void)
 {
     core::CScopedFastLock lock(m_Mutex);
 
-    if (m_IsBusy)
+    if (this->isBusy())
     {
         return false;
     }
@@ -168,7 +168,7 @@ bool CBackgroundPersister::firstProcessorPeriodicPersistFunc(const TFirstProcess
 {
     core::CScopedFastLock lock(m_Mutex);
 
-    if (m_IsBusy)
+    if (this->isBusy())
     {
         return false;
     }
@@ -232,7 +232,8 @@ CBackgroundPersister::CBackgroundThread::CBackgroundThread(CBackgroundPersister
 
 void CBackgroundPersister::CBackgroundThread::run(void)
 {
-
+    // The isBusy check will prevent concurrent access to
+    // m_Owner.m_PersistFuncs here
     while (!m_Owner.m_PersistFuncs.empty())
     {
         if (!m_Owner.m_IsShutdown)
@@ -243,7 +244,6 @@ void CBackgroundPersister::CBackgroundThread::run(void)
     }
 
     core::CScopedFastLock lock(m_Owner.m_Mutex);
-
     m_Owner.m_IsBusy = false;
 }