Merge branch 'master' into coverart_color_and_digest

3 files changed, 56 insertions, 33 deletions

diff --git a/src/track/beats.h b/src/track/beats.h
index 1ec09096d7..cdd155e97d 100644
--- a/src/track/beats.h
+++ b/src/track/beats.h
@@ -21,7 +21,7 @@ typedef QSharedPointer<Beats> BeatsPointer;
 
 class BeatIterator {
   public:
-    virtual ~BeatIterator() {}
+    virtual ~BeatIterator() = default;
     virtual bool hasNext() const = 0;
     virtual double next() = 0;
 };
@@ -33,7 +33,7 @@ class Beats : public QObject {
     Q_OBJECT
   public:
     Beats() { }
-    virtual ~Beats() { }
+    ~Beats() override = default;
 
     enum Capabilities {
         BEATSCAP_NONE          = 0x0000,
diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp
index 257c58af5b..e15df7026b 100644
--- a/src/track/globaltrackcache.cpp
+++ b/src/track/globaltrackcache.cpp
@@ -1,11 +1,10 @@
 #include "track/globaltrackcache.h"
 
-#include <QApplication>
-#include <QThread>
+#include <QCoreApplication>
 
 #include "util/assert.h"
 #include "util/logger.h"
-
+#include "util/thread_affinity.h"
 
 namespace {
 
@@ -209,7 +208,8 @@ void GlobalTrackCache::createInstance(
 
 //static
 void GlobalTrackCache::destroyInstance() {
-    DEBUG_ASSERT(s_pInstance);
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(s_pInstance);
+
     kLogger.info() << "Destroying instance";
     // Processing all pending events is required to evict all
     // remaining references from the cache.
@@ -220,7 +220,6 @@ void GlobalTrackCache::destroyInstance() {
     // Reset the static/global pointer before entering the destructor
     s_pInstance = nullptr;
     // Delete the singular instance
-    DEBUG_ASSERT(QThread::currentThread() == pInstance->thread());
     pInstance->deleteLater();
 }
 
@@ -261,20 +260,21 @@ void GlobalTrackCache::evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cach
         QMetaObject::invokeMethod(
                 s_pInstance,
 #if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
-                "evictAndSave"
+                "slotEvictAndSave",
 #else
                 [cacheEntryPtr = std::move(cacheEntryPtr)] {
-                    s_pInstance->evictAndSave(cacheEntryPtr);
-                }
+                    s_pInstance->slotEvictAndSave(cacheEntryPtr);
+                },
 #endif
                 // Qt will choose either a direct or a queued connection
                 // depending on the thread from which this method has
                 // been invoked!
-                , Qt::AutoConnection
+                Qt::AutoConnection
 #if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
-                , Q_ARG(GlobalTrackCacheEntryPointer, std::move(cacheEntryPtr))
+                ,
+                Q_ARG(GlobalTrackCacheEntryPointer, std::move(cacheEntryPtr))
 #endif
-                );
+        );
     } else {
         // After the singular instance has been destroyed we are
         // not able to save pending changes. The track is deleted
@@ -371,7 +371,7 @@ void GlobalTrackCache::saveEvictedTrack(Track* pEvictedTrack) const {
 }
 
 void GlobalTrackCache::deactivate() {
-    DEBUG_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread());
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
 
     if (isEmpty()) {
         return;
@@ -625,7 +625,7 @@ void GlobalTrackCache::resolve(
     // function might be called from any thread, even from worker
     // threads without an event loop. We need to move the newly
     // created object to the main thread.
-    savingPtr->moveToThread(QApplication::instance()->thread());
+    savingPtr->moveToThread(QCoreApplication::instance()->thread());
 
     pCacheResolver->initLookupResult(
             GlobalTrackCacheLookupResult::MISS,
@@ -670,16 +670,14 @@ void GlobalTrackCache::purgeTrackId(TrackId trackId) {
     }
 }
 
-void GlobalTrackCache::evictAndSave(
+void GlobalTrackCache::slotEvictAndSave(
         GlobalTrackCacheEntryPointer cacheEntryPtr) {
+    DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     DEBUG_ASSERT(cacheEntryPtr);
 
-    // We need to be sure this is always called from the main thread
-    // because we can only access the DB from it and we must not lose the
-    // the lock until all changes are persistently stored in file and DB
-    // to not hand out the track again with old metadata.
-    DEBUG_ASSERT(QApplication::instance()->thread() == QThread::currentThread());
-
+    // GlobalTrackCacheSaver::saveEvictedTrack() requires that
+    // exclusive access is guaranteed for the duration of the
+    // whole invocation!
     GlobalTrackCacheLocker cacheLocker;
 
     if (!cacheEntryPtr->expired()) {
@@ -708,8 +706,12 @@ void GlobalTrackCache::evictAndSave(
     DEBUG_ASSERT(!isCached(cacheEntryPtr->getPlainPtr()));
     saveEvictedTrack(cacheEntryPtr->getPlainPtr());
 
-    // here the cacheEntryPtr goes out of scope, the cache entry is
-    // deleted including the owned track
+    // Explicitly release the cacheEntryPtr including the owned
+    // track object while the cache is still locked.
+    cacheEntryPtr.reset();
+
+    // Finally the exclusive lock on the cache is released implicitly
+    // when exiting the scope of this method.
 }
 
 bool GlobalTrackCache::tryEvict(Track* plainPtr) {
diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h
index e7fb049ad4..7c3c170254 100644
--- a/src/track/globaltrackcache.h
+++ b/src/track/globaltrackcache.h
@@ -29,7 +29,7 @@ private:
             TrackFile fileInfo) = 0;
 
 protected:
-    virtual ~GlobalTrackCacheRelocator() {}
+  virtual ~GlobalTrackCacheRelocator() = default;
 };
 
 typedef void (*deleteTrackFn_t)(Track*);
@@ -174,19 +174,40 @@ private:
     TrackRef m_trackRef;
 };
 
+/// Callback interface for pre-delete actions
 class /*interface*/ GlobalTrackCacheSaver {
 private:
     friend class GlobalTrackCache;
-    virtual void saveEvictedTrack(Track* pEvictedTrack) noexcept = 0;
 
-protected:
-    virtual ~GlobalTrackCacheSaver() {}
+    /// Perform actions that are necessary to save any pending
+    /// modifications of a Track object before it finally gets
+    /// deleted.
+    ///
+    /// GlobalTrackCache ensures that the given pointer is valid
+    /// and the last and only reference to this Track object.
+    /// While invoked the GlobalTrackCache is locked to ensure
+    /// that this particular track is not accessible while
+    /// saving the Track object, e.g. by updating the database
+    /// and exporting file tags.
+    ///
+    /// This callback method will always be invoked from the
+    /// event loop thread of the owning GlobalTrackCache instance.
+    /// Typically the GlobalTrackCache lives on the main thread
+    /// that also controls access to the database.
+    /// NOTE(2020-06-06): If these assumptions about thread affinity
+    /// are no longer valid the design decisions need to be revisited
+    /// carefully!
+    virtual void saveEvictedTrack(
+            Track* pEvictedTrack) noexcept = 0;
+
+  protected:
+    virtual ~GlobalTrackCacheSaver() = default;
 };
 
 class GlobalTrackCache : public QObject {
     Q_OBJECT
 
-public:
+  public:
     static void createInstance(
             GlobalTrackCacheSaver* pSaver,
             // A custom deleter is only needed for tests without an event loop!
@@ -202,17 +223,17 @@ public:
     // Deleter callbacks for the smart-pointer
     static void evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cacheEntryPtr);
 
-private slots:
-    void evictAndSave(GlobalTrackCacheEntryPointer cacheEntryPtr);
+  private slots:
+    void slotEvictAndSave(GlobalTrackCacheEntryPointer cacheEntryPtr);
 
-private:
+  private:
     friend class GlobalTrackCacheLocker;
     friend class GlobalTrackCacheResolver;
 
     GlobalTrackCache(
             GlobalTrackCacheSaver* pSaver,
             deleteTrackFn_t deleteTrackFn);
-    ~GlobalTrackCache();
+    ~GlobalTrackCache() override;
 
     void relocateTracks(
             GlobalTrackCacheRelocator* /*nullable*/ pRelocator);