Merge pull request #2293 from uklotzde/2.2_globaltrackcache

GlobalTrackCache: Fix edge cases and tests

src/library/library.cpp

@@ -438,14 +438,11 @@ void Library::setEditMedatataSelectedClick(bool enabled) {
     emit(setSelectedClick(enabled));
 }
 
-void Library::saveCachedTrack(Track* pTrack) noexcept {
+void Library::saveEvictedTrack(Track* pTrack) noexcept {
     // It can produce dangerous signal loops if the track is still
     // sending signals while being saved!
     // See: https://bugs.launchpad.net/mixxx/+bug/1365708
-    // NOTE(uklotzde, 2018-02-03): Simply disconnecting all receivers
-    // doesn't seem to work reliably. Emitting the clean() signal from
-    // a track that is about to deleted may cause access violations!!
-    pTrack->blockSignals(true);
+    DEBUG_ASSERT(pTrack->signalsBlocked());
 
     // The metadata must be exported while the cache is locked to
     // ensure that we have exclusive (write) access on the file

src/library/library.h

@@ -126,7 +126,7 @@ class Library: public QObject,
 
   private:
     // Callback for GlobalTrackCache
-    void saveCachedTrack(Track* pTrack) noexcept override;
+    void saveEvictedTrack(Track* pTrack) noexcept override;
 
     const UserSettingsPointer m_pConfig;
 

src/test/globaltrackcache_test.cpp

@@ -27,8 +27,11 @@ class TrackTitleThread: public QThread {
 
     void run() override {
         int loopCount = 0;
-        while (!m_stop.load()) {
+        while (!(m_stop.load() && GlobalTrackCacheLocker().isEmpty())) {
+            // Drop the previous reference to avoid resolving the
+            // same track twice
             m_recentTrackPtr.reset();
+            // Try to resolve the next track by guessing the id
             const TrackId trackId(loopCount % 2);
             auto track = GlobalTrackCacheLocker().lookupTrackById(trackId);
             if (track) {
@@ -43,10 +46,17 @@ class TrackTitleThread: public QThread {
                 }
                 ASSERT_TRUE(track->isDirty());
             }
+            // Replace the current reference with this one and keep it alive
+            // until the next loop cycle
             m_recentTrackPtr = std::move(track);
             ++loopCount;
         }
-        m_recentTrackPtr.reset();
+        // If the cache is empty all references must have been dropped.
+        // Why? m_recentTrackPtr is only valid if a pointer has been found
+        // in the cache during the previous cycle, i.e. the cache could not
+        // have been empty. In this case at least another loop cycle follow,
+        // and so on...
+        ASSERT_TRUE(!m_recentTrackPtr);
         qDebug() << "Finished" << loopCount << " thread loops";
     }
 
@@ -56,17 +66,23 @@ class TrackTitleThread: public QThread {
     std::atomic<bool> m_stop;
 };
 
+void deleteTrack(Track* pTrack) {
+    // Delete track objects directly in unit tests with
+    // no main event loop
+    delete pTrack;
+};
+
 } // anonymous namespace
 
 class GlobalTrackCacheTest: public MixxxTest, public virtual GlobalTrackCacheSaver {
   public:
-    void saveCachedTrack(Track* pTrack) noexcept override {
+    void saveEvictedTrack(Track* pTrack) noexcept override {
         ASSERT_FALSE(pTrack == nullptr);
     }
 
   protected:
     GlobalTrackCacheTest() {
-        GlobalTrackCache::createInstance(this);
+        GlobalTrackCache::createInstance(this, deleteTrack);
     }
     ~GlobalTrackCacheTest() {
         GlobalTrackCache::destroyInstance();
@@ -156,19 +172,27 @@ TEST_F(GlobalTrackCacheTest, concurrentDelete) {
 
         // lp1744550: Accessing the track from multiple threads is
         // required to cause a SIGSEGV
-        track->setArtist(track->getTitle());
+        track->setArtist(QString("Artist %1").arg(QString::number(i)));
 
         m_recentTrackPtr = std::move(track);
+
+        // Lookup the track again
+        track = GlobalTrackCacheLocker().lookupTrackById(trackId);
+        EXPECT_TRUE(static_cast<bool>(track));
+
+        // Ensure that track objects are evicted and deleted
+        QCoreApplication::processEvents();
     }
     m_recentTrackPtr.reset();
 
     workerThread.stop();
-    workerThread.wait();
 
     // Ensure that all track objects have been deleted
-    QCoreApplication::processEvents();
+    while (!GlobalTrackCacheLocker().isEmpty()) {
+        QCoreApplication::processEvents();
+    }
 
-    EXPECT_TRUE(GlobalTrackCacheLocker().isEmpty());
+    workerThread.wait();
 }
 
 TEST_F(GlobalTrackCacheTest, evictWhileMoving) {
@@ -184,4 +208,8 @@ TEST_F(GlobalTrackCacheTest, evictWhileMoving) {
 
     EXPECT_TRUE(static_cast<bool>(track1));
     EXPECT_FALSE(static_cast<bool>(track2));
+
+    track1.reset();
+
+    EXPECT_TRUE(GlobalTrackCacheLocker().isEmpty());
 }

src/test/librarytest.cpp

@@ -0,0 +1,36 @@
+#include "test/librarytest.h"
+
+namespace {
+
+const bool kInMemoryDbConnection = true;
+
+void deleteTrack(Track* pTrack) {
+    // Delete track objects directly in unit tests with
+    // no main event loop
+    delete pTrack;
+};
+
+}
+
+LibraryTest::LibraryTest()
+    : m_mixxxDb(config(), kInMemoryDbConnection),
+      m_dbConnectionPooler(m_mixxxDb.connectionPool()),
+      m_dbConnection(mixxx::DbConnectionPooled(m_mixxxDb.connectionPool())),
+      m_pTrackCollection(std::make_unique<TrackCollection>(config())) {
+    MixxxDb::initDatabaseSchema(m_dbConnection);
+    m_pTrackCollection->connectDatabase(m_dbConnection);
+    GlobalTrackCache::createInstance(this, deleteTrack);
+}
+
+LibraryTest::~LibraryTest() {
+    m_pTrackCollection->disconnectDatabase();
+    m_pTrackCollection.reset();
+    // With the track collection all remaining track references
+    // should have been dropped before destroying the cache.
+    GlobalTrackCache::destroyInstance();
+}
+
+void LibraryTest::saveEvictedTrack(Track* pTrack) noexcept {
+    m_pTrackCollection->exportTrackMetadata(pTrack);
+    m_pTrackCollection->saveTrack(pTrack);
+}

src/test/librarytest.h

@@ -1,5 +1,6 @@
-#ifndef LIBRARYTEST_H
-#define LIBRARYTEST_H
+#pragma once
+
+#include <memory>
 
 #include "test/mixxxtest.h"
 
@@ -9,33 +10,15 @@
 #include "util/db/dbconnectionpooled.h"
 #include "track/globaltrackcache.h"
 
-namespace {
-    const bool kInMemoryDbConnection = true;
-} // anonymous namespace
-
 class LibraryTest : public MixxxTest,
     public virtual /*implements*/ GlobalTrackCacheSaver {
 
   public:
-    void saveCachedTrack(Track* pTrack) noexcept override {
-        m_trackCollection.exportTrackMetadata(pTrack);
-        m_trackCollection.saveTrack(pTrack);
-    }
+    void saveEvictedTrack(Track* pTrack) noexcept override;
 
   protected:
-    LibraryTest()
-        : m_mixxxDb(config(), kInMemoryDbConnection),
-          m_dbConnectionPooler(m_mixxxDb.connectionPool()),
-          m_dbConnection(mixxx::DbConnectionPooled(m_mixxxDb.connectionPool())),
-          m_trackCollection(config()) {
-        MixxxDb::initDatabaseSchema(m_dbConnection);
-        m_trackCollection.connectDatabase(m_dbConnection);
-        GlobalTrackCache::createInstance(this);
-    }
-    ~LibraryTest() override {
-        GlobalTrackCache::destroyInstance();
-        m_trackCollection.disconnectDatabase();
-    }
+    LibraryTest();
+    ~LibraryTest() override;
 
     mixxx::DbConnectionPoolPtr dbConnectionPool() const {
         return m_mixxxDb.connectionPool();
@@ -46,15 +29,12 @@ class LibraryTest : public MixxxTest,
     }
 
     TrackCollection* collection() {
-        return &m_trackCollection;
+        return m_pTrackCollection.get();
     }
 
   private:
     const MixxxDb m_mixxxDb;
     const mixxx::DbConnectionPooler m_dbConnectionPooler;
     QSqlDatabase m_dbConnection;
-    TrackCollection m_trackCollection;
+    std::unique_ptr<TrackCollection> m_pTrackCollection;
 };
-
-
-#endif /* LIBRARYTEST_H */