Skip to content

Commit

Permalink
Merge pull request #3027 from uklotzde/2.2_globaltrackcache
Browse files Browse the repository at this point in the history 
TrackDAO/GlobalTrackCache: Handle file aliasing
  • Loading branch information
@daschuer
daschuer authored Aug 25, 2020
2 parents e1d76c3 + 0f45c61 commit d2f3d77
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 59 deletions.
1 change: 1 addition & 0 deletions CHANGELOG
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Add controller mapping for Hercules DJControl Inpulse 200 #2542
* Add controller mapping for Hercules DJControl Jogvision #2370
* Fix missing manual in deb package lp:1889776
* Fix caching of duplicate tracks that reference the same file #3027

==== 2.2.4 2020-05-10 ====

Expand Down
44 changes: 36 additions & 8 deletions src/library/dao/trackdao.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,22 @@ TrackPointer TrackDAO::addTracksAddFile(const QFileInfo& fileInfo, bool unremove
qDebug() << "TrackDAO::addTracksAddFile:"
<< "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 (fileInfo.absoluteFilePath() != trackLocation) {
kLogger.warning()
<< "Cannot add track:"
<< "Both the new track at"
<< fileInfo.absoluteFilePath()
<< "and an existing track at"
<< trackLocation
<< "are referencing the same file"
<< fileInfo.canonicalFilePath();
return TrackPointer();
}
return pTrack;
}
// Keep the GlobalTrackCache locked until the id of the Track
Expand Down Expand Up @@ -1222,20 +1238,32 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
const QString trackLocation(queryRecord.value(0).toString());

GlobalTrackCacheResolver cacheResolver(QFileInfo(trackLocation), trackId);
TrackPointer 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) {
pTrack = cacheResolver.getTrack();
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();

Expand Down
135 changes: 98 additions & 37 deletions src/track/globaltrackcache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,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(
Expand Down Expand Up @@ -148,7 +175,7 @@ TrackPointer GlobalTrackCacheLocker::lookupTrackByRef(
GlobalTrackCacheResolver::GlobalTrackCacheResolver(
QFileInfo 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));
}
Expand All @@ -157,7 +184,7 @@ GlobalTrackCacheResolver::GlobalTrackCacheResolver(
QFileInfo 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));
}
Expand All @@ -167,7 +194,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);
Expand All @@ -176,7 +203,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()) {
Expand Down Expand Up @@ -391,8 +418,32 @@ bool GlobalTrackCache::isEmpty() const {
return m_tracksById.empty() && m_tracksByCanonicalLocation.empty();
}

TrackPointer GlobalTrackCache::lookupByRef(
const TrackRef& trackRef) {
TrackPointer trackPtr;
if (trackRef.hasId()) {
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 trackPtr;
}

TrackPointer GlobalTrackCache::lookupById(
const TrackId& trackId) {
TrackPointer trackPtr;
const auto trackById(m_tracksById.find(trackId));
if (m_tracksById.end() != trackById) {
// Cache hit
Expand All @@ -402,45 +453,43 @@ TrackPointer GlobalTrackCache::lookupById(
<< trackId
<< trackById->second->getPlainPtr();
}
return revive(trackById->second);
trackPtr = revive(trackById->second);
DEBUG_ASSERT(trackPtr);
} else {
// Cache miss
if (traceLogEnabled()) {
kLogger.trace()
<< "Cache miss for"
<< trackId;
}
return TrackPointer();
}
return trackPtr;
}

TrackPointer GlobalTrackCache::lookupByRef(
const TrackRef& trackRef) {
if (trackRef.hasId()) {
return lookupById(trackRef.getId());
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 {
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;
}
return TrackPointer();
// Cache miss
if (traceLogEnabled()) {
kLogger.trace()
<< "Cache miss for"
<< canonicalLocation;
}
}
return trackPtr;
}

TrackPointer GlobalTrackCache::revive(
Expand Down Expand Up @@ -499,7 +548,7 @@ void GlobalTrackCache::resolve(
}
TrackRef trackRef = createTrackRef(*strongPtr);
pCacheResolver->initLookupResult(
GlobalTrackCacheLookupResult::HIT,
GlobalTrackCacheLookupResult::Hit,
std::move(strongPtr),
std::move(trackRef));
return;
Expand All @@ -515,7 +564,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()) {
Expand All @@ -524,10 +574,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;
}
}
Expand Down Expand Up @@ -591,7 +652,7 @@ void GlobalTrackCache::resolve(
savingPtr->moveToThread(QApplication::instance()->thread());

pCacheResolver->initLookupResult(
GlobalTrackCacheLookupResult::MISS,
GlobalTrackCacheLookupResult::Miss,
std::move(savingPtr),
std::move(trackRef));
}
Expand Down
14 changes: 11 additions & 3 deletions src/track/globaltrackcache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -228,6 +229,13 @@ private slots:

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);

Expand Down
31 changes: 20 additions & 11 deletions src/track/trackref.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "track/trackref.h"

#include <QDebugStateSaver>

bool TrackRef::verifyConsistency() const {
// Class invariant: The location can only be set together with
Expand All @@ -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()
<< '}';
}

0 comments on commit d2f3d77

Please sign in to comment.