diff options
author | Uwe Klotz <uklotz@mixxx.org> | 2020-08-25 11:34:00 +0200 |
---|---|---|
committer | Uwe Klotz <uklotz@mixxx.org> | 2020-08-25 11:34:00 +0200 |
commit | e6ccf16919ee9650d845d8e5023e403a46756886 (patch) | |
tree | eeab651dd72566fe70eb0103b2d50562eddfa316 | |
parent | 0a693b689a429582e3fa43ccce86b33465794e9f (diff) | |
parent | d2f3d77fae2074dc6e8d3baed4bda42bfe49a418 (diff) |
Merge branch '2.2' of git@github.com:mixxxdj/mixxx.git into 2.3
# Conflicts:
# CHANGELOG
# src/library/dao/trackdao.cpp
-rw-r--r-- | CHANGELOG.md | 1 | ||||
-rw-r--r-- | src/library/dao/trackdao.cpp | 41 | ||||
-rw-r--r-- | src/track/globaltrackcache.cpp | 129 | ||||
-rw-r--r-- | src/track/globaltrackcache.h | 15 | ||||
-rw-r--r-- | src/track/trackref.cpp | 31 |
5 files changed, 162 insertions, 55 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index a0535686cf..bde0fe17fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ * Add controller mapping for Hercules DJControl Inpulse 200 [#2542](https://github.com/mixxxdj/mixxx/pull/2542) * Add controller mapping for Hercules DJControl Jogvision [#2370](https://github.com/mixxxdj/mixxx/pull/2370) * Fix missing manual in deb package [lp:1889776] (https://bugs.launchpad.net/mixxx/+bug/1889776) +* Fix caching of duplicate tracks that reference the same file #3027 ## [2.2.4](https://launchpad.net/mixxx/+milestone/2.2.4) (2020-06-27) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 6aced66316..c58e893ac9 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -678,6 +678,21 @@ TrackPointer TrackDAO::addTracksAddFile(const TrackFile& trackFile, bool unremov << "Track has already been added to the database" << oldTrackId; DEBUG_ASSERT(pTrack->getDateAdded().isValid()); + const auto trackLocation = pTrack->getLocation(); + // TODO: These duplicates are only detected by chance when + // the other track is currently cached. Instead file aliasing + // must be detected reliably in any situation. + if (trackFile.location() != trackLocation) { + kLogger.warning() + << "Cannot add track:" + << "Both the new track at" + << trackFile.location() + << "and an existing track at" + << trackLocation + << "are referencing the same file" + << trackFile.canonicalLocation(); + return TrackPointer(); + } return pTrack; } // Keep the GlobalTrackCache locked until the id of the Track @@ -1241,19 +1256,31 @@ TrackPointer TrackDAO::getTrackById(TrackId trackId) const { GlobalTrackCacheResolver cacheResolver(TrackFile(trackLocation), trackId); pTrack = cacheResolver.getTrack(); - VERIFY_OR_DEBUG_ASSERT(pTrack) { - // Just to be safe, but this should never happen!! - return pTrack; - } - DEBUG_ASSERT(pTrack->getId() == trackId); - if (cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::HIT) { + if (cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::Hit) { // Due to race conditions the track might have been reloaded // from the database in the meantime. In this case we abort // the operation and simply return the already cached Track // object which is up-to-date. + DEBUG_ASSERT(pTrack); + return pTrack; + } + if (cacheResolver.getLookupResult() == + GlobalTrackCacheLookupResult::ConflictCanonicalLocation) { + // Reject requests that would otherwise cause a caching caching conflict + // by accessing the same, physical file from multiple tracks concurrently. + DEBUG_ASSERT(!pTrack); + DEBUG_ASSERT(cacheResolver.getTrackRef().hasId()); + DEBUG_ASSERT(cacheResolver.getTrackRef().hasCanonicalLocation()); + kLogger.warning() + << "Failed to load track with id" + << trackId + << "that is referencing the same file" + << cacheResolver.getTrackRef().getCanonicalLocation() + << "as the cached track with id" + << cacheResolver.getTrackRef().getId(); return pTrack; } - DEBUG_ASSERT(cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::MISS); + DEBUG_ASSERT(cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::Miss); // The cache will immediately be unlocked to reduce lock contention! cacheResolver.unlockCache(); diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp index e15df7026b..e2da4c0456 100644 --- a/src/track/globaltrackcache.cpp +++ b/src/track/globaltrackcache.cpp @@ -33,6 +33,33 @@ TrackRef createTrackRef(const Track& track) { return TrackRef::fromFileInfo(track.getFileInfo(), track.getId()); } +TrackRef validateAndCanonicalizeRequestedTrackRef( + const TrackRef requestedTrackRef, + const Track& cachedTrack) { + const auto cachedTrackRef = createTrackRef(cachedTrack); + // If an id has been provided the caller expects that if a track + // is found it is supposed to have the exact same id. This cannot + // be guaranteed due to file system aliasing. + // The found track may or may not have a valid id. + if (requestedTrackRef.hasId() && + requestedTrackRef.getId() != cachedTrackRef.getId()) { + DEBUG_ASSERT( + requestedTrackRef.getLocation() != + cachedTrackRef.getLocation()); + DEBUG_ASSERT( + requestedTrackRef.getCanonicalLocation() == + cachedTrackRef.getCanonicalLocation()); + kLogger.warning() + << "Found a different track for the same canonical location:" + << "requested =" << requestedTrackRef + << "cached =" << cachedTrackRef; + return cachedTrackRef; + } else { + // Regular case, i.e. no aliasing + return requestedTrackRef; + } +} + class EvictAndSaveFunctor { public: explicit EvictAndSaveFunctor( @@ -152,7 +179,7 @@ QSet<TrackId> GlobalTrackCacheLocker::getCachedTrackIds() const { GlobalTrackCacheResolver::GlobalTrackCacheResolver( TrackFile fileInfo, SecurityTokenPointer pSecurityToken) - : m_lookupResult(GlobalTrackCacheLookupResult::NONE) { + : m_lookupResult(GlobalTrackCacheLookupResult::None) { DEBUG_ASSERT(m_pInstance); m_pInstance->resolve(this, std::move(fileInfo), TrackId(), std::move(pSecurityToken)); } @@ -161,7 +188,7 @@ GlobalTrackCacheResolver::GlobalTrackCacheResolver( TrackFile fileInfo, TrackId trackId, SecurityTokenPointer pSecurityToken) - : m_lookupResult(GlobalTrackCacheLookupResult::NONE) { + : m_lookupResult(GlobalTrackCacheLookupResult::None) { DEBUG_ASSERT(m_pInstance); m_pInstance->resolve(this, std::move(fileInfo), std::move(trackId), std::move(pSecurityToken)); } @@ -171,7 +198,7 @@ void GlobalTrackCacheResolver::initLookupResult( TrackPointer&& strongPtr, TrackRef&& trackRef) { DEBUG_ASSERT(m_pInstance); - DEBUG_ASSERT(GlobalTrackCacheLookupResult::NONE == m_lookupResult); + DEBUG_ASSERT(GlobalTrackCacheLookupResult::None == m_lookupResult); DEBUG_ASSERT(!m_strongPtr); m_lookupResult = lookupResult; m_strongPtr = std::move(strongPtr); @@ -180,7 +207,7 @@ void GlobalTrackCacheResolver::initLookupResult( void GlobalTrackCacheResolver::initTrackIdAndUnlockCache(TrackId trackId) { DEBUG_ASSERT(m_pInstance); - DEBUG_ASSERT(GlobalTrackCacheLookupResult::NONE != m_lookupResult); + DEBUG_ASSERT(GlobalTrackCacheLookupResult::None != m_lookupResult); DEBUG_ASSERT(m_strongPtr); DEBUG_ASSERT(trackId.isValid()); if (m_trackRef.getId().isValid()) { @@ -422,6 +449,7 @@ bool GlobalTrackCache::isEmpty() const { TrackPointer GlobalTrackCache::lookupById( const TrackId& trackId) { + TrackPointer trackPtr; const auto trackById(m_tracksById.find(trackId)); if (m_tracksById.end() != trackById) { // Cache hit @@ -431,7 +459,8 @@ TrackPointer GlobalTrackCache::lookupById( << trackId << trackById->second->getPlainPtr(); } - return revive(trackById->second); + trackPtr = revive(trackById->second); + DEBUG_ASSERT(trackPtr); } else { // Cache miss if (traceLogEnabled()) { @@ -439,37 +468,57 @@ TrackPointer GlobalTrackCache::lookupById( << "Cache miss for" << trackId; } - return TrackPointer(); } + return trackPtr; } TrackPointer GlobalTrackCache::lookupByRef( const TrackRef& trackRef) { + TrackPointer trackPtr; if (trackRef.hasId()) { - return lookupById(trackRef.getId()); - } else { - const auto canonicalLocation = trackRef.getCanonicalLocation(); - const auto trackByCanonicalLocation( - m_tracksByCanonicalLocation.find(canonicalLocation)); - if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) { - // Cache hit - if (traceLogEnabled()) { - kLogger.trace() - << "Cache hit for" - << canonicalLocation - << trackByCanonicalLocation->second->getPlainPtr(); - } - return revive(trackByCanonicalLocation->second); - } else { - // Cache miss - if (traceLogEnabled()) { - kLogger.trace() - << "Cache miss for" - << canonicalLocation; + trackPtr = lookupById(trackRef.getId()); + if (trackPtr) { + return trackPtr; + } + } + if (trackRef.hasCanonicalLocation()) { + trackPtr = lookupByCanonicalLocation(trackRef.getCanonicalLocation()); + if (trackPtr) { + const auto cachedTrackRef = + validateAndCanonicalizeRequestedTrackRef(trackRef, *trackPtr); + // Multiple tracks may reference the same physical file on disk + if (!trackRef.hasId() || trackRef.getId() == cachedTrackRef.getId()) { + return trackPtr; } - return TrackPointer(); } } + return trackPtr; +} + +TrackPointer GlobalTrackCache::lookupByCanonicalLocation( + const QString& canonicalLocation) { + TrackPointer trackPtr; + const auto trackByCanonicalLocation( + m_tracksByCanonicalLocation.find(canonicalLocation)); + if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) { + // Cache hit + if (traceLogEnabled()) { + kLogger.trace() + << "Cache hit for" + << canonicalLocation + << trackByCanonicalLocation->second->getPlainPtr(); + } + trackPtr = revive(trackByCanonicalLocation->second); + DEBUG_ASSERT(trackPtr); + } else { + // Cache miss + if (traceLogEnabled()) { + kLogger.trace() + << "Cache miss for" + << canonicalLocation; + } + } + return trackPtr; } QSet<TrackId> GlobalTrackCache::getCachedTrackIds() const { @@ -536,7 +585,7 @@ void GlobalTrackCache::resolve( } TrackRef trackRef = createTrackRef(*strongPtr); pCacheResolver->initLookupResult( - GlobalTrackCacheLookupResult::HIT, + GlobalTrackCacheLookupResult::Hit, std::move(strongPtr), std::move(trackRef)); return; @@ -552,7 +601,8 @@ void GlobalTrackCache::resolve( << "Resolving track by canonical location" << trackRef.getCanonicalLocation(); } - auto strongPtr = lookupByRef(trackRef); + auto strongPtr = lookupByCanonicalLocation( + trackRef.getCanonicalLocation()); if (strongPtr) { // Cache hit if (debugLogEnabled()) { @@ -561,10 +611,21 @@ void GlobalTrackCache::resolve( << trackRef.getCanonicalLocation() << strongPtr.get(); } - pCacheResolver->initLookupResult( - GlobalTrackCacheLookupResult::HIT, - std::move(strongPtr), - std::move(trackRef)); + auto cachedTrackRef = validateAndCanonicalizeRequestedTrackRef( + trackRef, + *strongPtr); + // Multiple tracks may reference the same physical file on disk + if (!trackRef.hasId() || trackRef.getId() == cachedTrackRef.getId()) { + pCacheResolver->initLookupResult( + GlobalTrackCacheLookupResult::Hit, + std::move(strongPtr), + std::move(trackRef)); + } else { + pCacheResolver->initLookupResult( + GlobalTrackCacheLookupResult::ConflictCanonicalLocation, + TrackPointer(), + std::move(cachedTrackRef)); + } return; } } @@ -628,7 +689,7 @@ void GlobalTrackCache::resolve( savingPtr->moveToThread(QCoreApplication::instance()->thread()); pCacheResolver->initLookupResult( - GlobalTrackCacheLookupResult::MISS, + GlobalTrackCacheLookupResult::Miss, std::move(savingPtr), std::move(trackRef)); } diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h index 7c3c170254..6266a230ee 100644 --- a/src/track/globaltrackcache.h +++ b/src/track/globaltrackcache.h @@ -12,9 +12,10 @@ class GlobalTrackCache; enum class GlobalTrackCacheLookupResult { - NONE, - HIT, - MISS + None, + Hit, + Miss, + ConflictCanonicalLocation }; // Find the updated location of a track in the database when @@ -240,8 +241,16 @@ class GlobalTrackCache : public QObject { TrackPointer lookupById( const TrackId& trackId); + TrackPointer lookupByCanonicalLocation( + const QString& canonicalLocation); + + /// Lookup the track either by id (primary) or by + /// canonical location (secondary). The id of the + /// returned track might differ from the requested + /// id due to file system aliasing!! TrackPointer lookupByRef( const TrackRef& trackRef); + QSet<TrackId> getCachedTrackIds() const; TrackPointer revive(GlobalTrackCacheEntryPointer entryPtr); diff --git a/src/track/trackref.cpp b/src/track/trackref.cpp index 663a2f9790..60d48911f1 100644 --- a/src/track/trackref.cpp +++ b/src/track/trackref.cpp @@ -1,5 +1,6 @@ #include "track/trackref.h" +#include <QDebugStateSaver> bool TrackRef::verifyConsistency() const { // Class invariant: The location can only be set together with @@ -16,17 +17,25 @@ bool TrackRef::verifyConsistency() const { } std::ostream& operator<<(std::ostream& os, const TrackRef& trackRef) { - return os << '[' << trackRef.getLocation().toStdString() - << " | " << trackRef.getCanonicalLocation().toStdString() - << " | " << trackRef.getId() - << ']'; - + return os + << "TrackRef{" + << trackRef.getLocation().toStdString() + << ',' + << trackRef.getCanonicalLocation().toStdString() + << ',' + << trackRef.getId() + << '}'; } -QDebug operator<<(QDebug debug, const TrackRef& trackRef) { - debug.nospace() << '[' << trackRef.getLocation() - << " | " << trackRef.getCanonicalLocation() - << " | " << trackRef.getId() - << ']'; - return debug.space(); +QDebug operator<<(QDebug dbg, const TrackRef& trackRef) { + const QDebugStateSaver saver(dbg); + dbg = dbg.maybeSpace() << "TrackRef"; + return dbg.nospace() + << '{' + << trackRef.getLocation() + << ',' + << trackRef.getCanonicalLocation() + << ',' + << trackRef.getId() + << '}'; } |