tsan fix browsethread

src/library/browse/browsetablemodel.cpp

@@ -211,7 +211,8 @@ void BrowseTableModel::setPath(mixxx::FileAccess path) {
     } else {
         m_currentDirectory = QString();
     }
-    m_pBrowseThread->executePopulation(std::move(path), this);
+    // Request population of this model by the browse thread
+    m_pBrowseThread->requestPopulateModel(std::move(path), this);
 }
 
 TrackPointer BrowseTableModel::getTrack(const QModelIndex& index) const {

src/library/browse/browsethread.cpp

@@ -14,80 +14,121 @@
 constexpr int kRowBatchSize = 10;
 } // namespace
 
-QWeakPointer<BrowseThread> BrowseThread::m_weakInstanceRef;
-static QMutex s_Mutex;
-
 /*
  * This class is a singleton and represents a thread
  * that is used to read ID3 metadata
  * from a particular folder.
  *
  * The BrowseTableModel uses this class.
- * Note: Don't call getInstance() from places
+ * Note: Don't call getInstanceRef() from places
  * other than the GUI thread. BrowseThreads emit
  * signals to BrowseModel objects. It does not
  * make sense to use this class in non-GUI threads
  */
-BrowseThread::BrowseThread(QObject *parent)
-        : QThread(parent) {
-    m_bStopThread = false;
-    m_model_observer = nullptr;
-    //start Thread
+BrowseThread::BrowseThread(QObject* parent)
+        : QThread{parent},
+          m_pModelObserver{},
+          m_runState{} {
+    qDebug() << "Starting browser background thread";
     start(QThread::LowPriority);
-
+    qDebug() << "Wait for browser background thread start";
+    waitUntilThreadIsRunning();
+    qDebug() << "Browser background thread has started";
 }
 
 BrowseThread::~BrowseThread() {
-    qDebug() << "Wait to finish browser background thread";
-    m_bStopThread = true;
-    //wake up thread since it might wait for user input
-    m_locationUpdated.wakeAll();
-    //Wait until thread terminated
-    //terminate();
+    qDebug() << "Request to terminate browser background thread";
+    requestThreadToStop();
+    qDebug() << "Wait for browser background thread to finish";
     wait();
-    qDebug() << "Browser background thread terminated!";
+    qDebug() << "Browser background thread terminated";
+}
+
+void BrowseThread::waitUntilThreadIsRunning() {
+    m_requestMutex.lock();
+    while (!m_runState) {
+        m_requestCondition.wait(&m_requestMutex);
+    }
+    m_requestMutex.unlock();
+}
+
+void BrowseThread::requestThreadToStop() {
+    m_requestMutex.lock();
+    m_runState = false;
+    m_requestMutex.unlock();
+    m_requestCondition.wakeOne();
 }
 
 // static
 BrowseThreadPointer BrowseThread::getInstanceRef() {
-    BrowseThreadPointer strong = m_weakInstanceRef.toStrongRef();
+    // We create a single BrowseThread instead which is used by multiple
+    // BrowseTableModel instances. We store a weakpointer so when all
+    // BrowseTableModel have been destroyed, so is the BrowseThread.
+
+    static QWeakPointer<BrowseThread> s_weakInstanceRef;
+    static QMutex s_mutex;
+
+    BrowseThreadPointer strong = s_weakInstanceRef.toStrongRef();
+
     if (!strong) {
-        s_Mutex.lock();
-        strong = m_weakInstanceRef.toStrongRef();
+        s_mutex.lock();
+        strong = s_weakInstanceRef.toStrongRef();
         if (!strong) {
             strong = BrowseThreadPointer(new BrowseThread());
-            m_weakInstanceRef = strong.toWeakRef();
+            s_weakInstanceRef = strong.toWeakRef();
         }
-        s_Mutex.unlock();
+        s_mutex.unlock();
     }
+
     return strong;
 }
 
-void BrowseThread::executePopulation(mixxx::FileAccess path, BrowseTableModel* client) {
-    m_path_mutex.lock();
-    m_path = std::move(path);
-    m_model_observer = client;
-    m_path_mutex.unlock();
-    m_locationUpdated.wakeAll();
+void BrowseThread::requestPopulateModel(mixxx::FileAccess path, BrowseTableModel* pModelObserver) {
+    // Inform the thread to populate a new path.
+    // Note: if another request is currently being processed, this will replace it.
+
+    qDebug() << "Request populate model" << path.info() << pModelObserver;
+    m_requestMutex.lock();
+    m_requestedPath = std::move(path);
+    m_pModelObserver = pModelObserver;
+    m_requestMutex.unlock();
+    m_requestCondition.wakeOne();
 }
 
 void BrowseThread::run() {
     QThread::currentThread()->setObjectName("BrowseThread");
-    m_mutex.lock();
 
-    while (!m_bStopThread) {
-        //Wait until the user has selected a folder
-        m_locationUpdated.wait(&m_mutex);
+    // Inform the constructor the thread started
+    m_requestMutex.lock();
+    m_runState = true;
+    m_requestMutex.unlock();
+    m_requestCondition.wakeOne();
+
+    while (true) {
+        // Wait for a new population request, or for a termination request
+        qDebug() << "Wait for a new population request";
+        m_requestMutex.lock();
+        while (!m_requestedPath.isSet() && m_runState) {
+            m_requestCondition.wait(&m_requestMutex);
+        }
+        auto path = std::move(m_requestedPath);
+        auto pModelObserver = m_pModelObserver;
+        auto bRun = m_runState;
+        m_requestedPath = mixxx::FileAccess();
+        m_requestMutex.unlock();
+
         Trace trace("BrowseThread");
 
-        //Terminate thread if Mixxx closes
-        if(m_bStopThread) {
-            break;
+        if (!bRun) {
+            qDebug() << "Termination request";
+            // Terminate thread if Mixxx closes
+            return;
+        } else {
+            qDebug() << "New population request";
+            // Populate the model
+            populateModel(path, pModelObserver);
         }
-        // Populate the model
-        populateModel();
     }
-    m_mutex.unlock();
 }
 
 namespace {
@@ -112,29 +153,27 @@
 
 } // namespace
 
-void BrowseThread::populateModel() {
-    m_path_mutex.lock();
-    auto thisPath = m_path;
-    BrowseTableModel* thisModelObserver = m_model_observer;
-    m_path_mutex.unlock();
+void BrowseThread::populateModel(const mixxx::FileAccess& path, BrowseTableModel* pModelObserver) {
+    // Executed by the thread run for incoming populate model requests
 
-    if (!thisPath.info().hasLocation()) {
+    if (!path.info().hasLocation()) {
         // Abort if the location is inaccessible or does not exist
-        qWarning() << "Skipping" << thisPath.info();
+        qWarning() << "Skipping" << path.info();
         return;
     }
+    qDebug() << "populateModel" << path.info() << pModelObserver;
 
     // Refresh the name filters in case we loaded new SoundSource plugins.
     const QStringList nameFilters = SoundSourceProxy::getSupportedFileNamePatterns();
 
-    QDirIterator fileIt(thisPath.info().location(),
+    QDirIterator fileIt(path.info().location(),
             nameFilters,
             QDir::Files | QDir::NoDotAndDotDot);
 
     // remove all rows
     // This is a blocking operation
     // see signal/slot connection in BrowseTableModel
-    emit clearModel(thisModelObserver);
+    emit clearModel(pModelObserver);
 
     QList<QList<QStandardItem*>> rows;
     rows.reserve(kRowBatchSize);
@@ -144,15 +183,19 @@
     int row = 0;
     // Iterate over the files
     while (fileIt.hasNext()) {
-        // If a user quickly jumps through the folders
-        // the current task becomes "dirty"
-        m_path_mutex.lock();
-        auto newPath = m_path;
-        m_path_mutex.unlock();
-
-        if (thisPath.info() != newPath.info()) {
-            qDebug() << "Abort populateModel()";
-            populateModel();
+        // If while processing a new population request or a termination request
+        // was received, we abort the current request. This happens if a user
+        // quickly jumps through the folders and the current task becomes
+        // "dirty".
+        m_requestMutex.lock();
+        const bool abortProcess = !m_runState ||
+                (m_requestedPath.isSet() &&
+                        (m_requestedPath.info() != path.info() ||
+                                m_pModelObserver != pModelObserver));
+        m_requestMutex.unlock();
+        if (abortProcess) {
+            qDebug() << "Abort populateModel";
+            // The run function will take care of the new request.
             return;
         }
 
@@ -162,7 +205,7 @@
 
         const auto fileAccess = mixxx::FileAccess(
                 mixxx::FileInfo(fileIt.next()),
-                thisPath.token());
+                path.token());
         {
             mixxx::TrackMetadata trackMetadata;
             // Both resetMissingTagMetadata = false/true have the same effect
@@ -302,14 +345,14 @@
         // Will limit GUI freezing
         if (row % kRowBatchSize == 0) {
             // this is a blocking operation
-            emit rowsAppended(rows, thisModelObserver);
+            emit rowsAppended(rows, pModelObserver);
             qDebug() << "Append" << rows.count() << "tracks from "
-                     << thisPath.info().locationPath();
+                     << path.info().locationPath();
             rows.clear();
         }
         // Sleep additionally for 20ms which prevents us from GUI freezes
         msleep(20);
     }
-    emit rowsAppended(rows, thisModelObserver);
-    qDebug() << "Append last" << rows.count() << "tracks from" << thisPath.info().locationPath();
+    emit rowsAppended(rows, pModelObserver);
+    qDebug() << "Append last" << rows.count() << "tracks from" << path.info().locationPath();
 }

src/library/browse/browsethread.h

@@ -30,7 +30,7 @@ class BrowseThread : public QThread {
     Q_OBJECT
   public:
     virtual ~BrowseThread();
-    void executePopulation(mixxx::FileAccess path, BrowseTableModel* client);
+    void requestPopulateModel(mixxx::FileAccess path, BrowseTableModel* pModelObserver);
     void run();
     static BrowseThreadPointer getInstanceRef();
 
@@ -41,16 +41,16 @@ class BrowseThread : public QThread {
   private:
     BrowseThread(QObject *parent = 0);
 
-    void populateModel();
+    void waitUntilThreadIsRunning();
+    void requestThreadToStop();
+    void populateModel(const mixxx::FileAccess& path, BrowseTableModel* pModelObserver);
 
-    QMutex m_mutex;
-    QWaitCondition m_locationUpdated;
-    volatile bool m_bStopThread;
+    QMutex m_requestMutex;
+    QWaitCondition m_requestCondition;
 
-    // You must hold m_path_mutex to touch m_path or m_model_observer
-    QMutex m_path_mutex;
-    mixxx::FileAccess m_path;
-    BrowseTableModel* m_model_observer;
-
-    static QWeakPointer<BrowseThread> m_weakInstanceRef;
+    // You must hold m_requestMutex to touch m_requestedPath, m_pModelObserver
+    // or m_runState
+    mixxx::FileAccess m_requestedPath;
+    BrowseTableModel* m_pModelObserver;
+    bool m_runState;
 };

src/util/fileaccess.h

@@ -19,6 +19,10 @@ class FileAccess final {
     FileAccess& operator=(FileAccess&&) = default;
     FileAccess& operator=(const FileAccess&) = default;
 
+    bool isSet() const {
+        return !m_fileInfo.asQFileInfo().filePath().isEmpty();
+    }
+
     const FileInfo& info() const {
         return m_fileInfo;
     }