From f4426d730de8c766aa9fa4067e130c555c622974 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 16 Aug 2020 19:22:41 +0200 Subject: TrackDAO: Handle and report aliasing of track files --- src/library/dao/trackdao.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) (limited to 'src/library/dao') diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index db20efa36f..955983ca81 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1227,7 +1227,20 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const { // Just to be safe, but this should never happen!! return pTrack; } - DEBUG_ASSERT(pTrack->getId() == trackId); + if (pTrack->getId() != trackId) { + // This happens if two different tracks are referencing + // the same (physical) file on the file system!! Due to + // symbolic links different locations may resolve to the + // same canonical location. We can only load each file + // once at a time. + kLogger.warning() + << "Returning already loaded track" + << pTrack->getId() + << "instead of" + << trackId + << "with the same canonical file location" + << pTrack->getFileInfo().canonicalFilePath(); + } 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 -- cgit v1.2.3 From 90d04d495940a6312b3e181b6bfdc16909f0af39 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 17 Aug 2020 00:14:46 +0200 Subject: Clarify ambiguities caused by file system aliasing --- src/library/dao/trackdao.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'src/library/dao') diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 955983ca81..3b9ee98f25 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1228,11 +1228,13 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const { return pTrack; } if (pTrack->getId() != trackId) { - // This happens if two different tracks are referencing - // the same (physical) file on the file system!! Due to - // symbolic links different locations may resolve to the - // same canonical location. We can only load each file - // once at a time. + // The cacheResolver() failed to resolve the track by it's trackId, + // but has found a different track that has already been loaded and + // is stored in the cache. That track references the same (physical) + // file on the file system!! Due to symbolic links different locations + // may resolve to the same canonical location. We can only load and + // access each file once at a time to prevent corruption due to + // concurrent, non-exclusive (write) access. kLogger.warning() << "Returning already loaded track" << pTrack->getId() -- cgit v1.2.3 From aa4d2d66481a3a57f2772b4539180dc491ffff12 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 19 Aug 2020 15:59:06 +0200 Subject: Refuse to load tracks with conflicting canonical locations --- src/library/dao/trackdao.cpp | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) (limited to 'src/library/dao') diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 3b9ee98f25..ad65bc0ec8 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1222,35 +1222,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; - } - if (pTrack->getId() != trackId) { - // The cacheResolver() failed to resolve the track by it's trackId, - // but has found a different track that has already been loaded and - // is stored in the cache. That track references the same (physical) - // file on the file system!! Due to symbolic links different locations - // may resolve to the same canonical location. We can only load and - // access each file once at a time to prevent corruption due to - // concurrent, non-exclusive (write) access. - kLogger.warning() - << "Returning already loaded track" - << pTrack->getId() - << "instead of" - << trackId - << "with the same canonical file location" - << pTrack->getFileInfo().canonicalFilePath(); - } - 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(); -- cgit v1.2.3 From 843c19f28530b6ffc340d4f742393545a553bf2c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 19 Aug 2020 17:06:15 +0200 Subject: Refuse to add tracks with conflicting canonical locations --- src/library/dao/trackdao.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'src/library/dao') diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index ad65bc0ec8..b2ad5c9290 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -702,6 +702,21 @@ 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() + << "Both track locations" + << fileInfo.absoluteFilePath() + << "and" + << trackLocation + << "are referencing the same file" + << fileInfo.canonicalFilePath(); + return TrackPointer(); + } return pTrack; } // Keep the GlobalTrackCache locked until the id of the Track -- cgit v1.2.3 From 0f45c61d68aba5acf259b1d7c500dc53f8b3c336 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 19 Aug 2020 22:57:08 +0200 Subject: Improve log message --- src/library/dao/trackdao.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src/library/dao') diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index b2ad5c9290..4af5ebe93e 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -709,9 +709,10 @@ TrackPointer TrackDAO::addTracksAddFile(const QFileInfo& fileInfo, bool unremove // must be detected reliably in any situation. if (fileInfo.absoluteFilePath() != trackLocation) { kLogger.warning() - << "Both track locations" + << "Cannot add track:" + << "Both the new track at" << fileInfo.absoluteFilePath() - << "and" + << "and an existing track at" << trackLocation << "are referencing the same file" << fileInfo.canonicalFilePath(); -- cgit v1.2.3