Fix crash when cache File lifetime exceeds cache lifetime

libraries/model-networking/src/model-networking/KTXCache.cpp

@@ -24,9 +24,10 @@ const int KTXCache::INVALID_VERSION = 0x00;
 const char* KTXCache::SETTING_VERSION_NAME = "hifi.ktx.cache_version";
 
 KTXCache::KTXCache(const std::string& dir, const std::string& ext) :
-    FileCache(dir, ext) {
-    initialize();
+    FileCache(dir, ext) { }
 
+void KTXCache::initialize() {
+    FileCache::initialize();
     Setting::Handle<int> cacheVersionHandle(SETTING_VERSION_NAME, INVALID_VERSION);
     auto cacheVersion = cacheVersionHandle.get();
     if (cacheVersion != CURRENT_VERSION) {
@@ -35,20 +36,9 @@ KTXCache::KTXCache(const std::string& dir, const std::string& ext) :
     }
 }
 
-KTXFilePointer KTXCache::writeFile(const char* data, Metadata&& metadata) {
-    FilePointer file = FileCache::writeFile(data, std::move(metadata), true);
-    return std::static_pointer_cast<KTXFile>(file);
-}
-
-KTXFilePointer KTXCache::getFile(const Key& key) {
-    return std::static_pointer_cast<KTXFile>(FileCache::getFile(key));
-}
 
 std::unique_ptr<File> KTXCache::createFile(Metadata&& metadata, const std::string& filepath) {
     qCInfo(file_cache) << "Wrote KTX" << metadata.key.c_str();
-    return std::unique_ptr<File>(new KTXFile(std::move(metadata), filepath));
+    return FileCache::createFile(std::move(metadata), filepath);
 }
 
-KTXFile::KTXFile(Metadata&& metadata, const std::string& filepath) :
-    cache::File(std::move(metadata), filepath) {}
-

libraries/model-networking/src/model-networking/KTXCache.h

@@ -35,20 +35,10 @@ public:
 
     KTXCache(const std::string& dir, const std::string& ext);
 
-    KTXFilePointer writeFile(const char* data, Metadata&& metadata);
-    KTXFilePointer getFile(const Key& key);
+    void initialize() override;
 
 protected:
     std::unique_ptr<cache::File> createFile(Metadata&& metadata, const std::string& filepath) override final;
 };
 
-class KTXFile : public cache::File {
-    Q_OBJECT
-
-protected:
-    friend class KTXCache;
-
-    KTXFile(Metadata&& metadata, const std::string& filepath);
-};
-
 #endif // hifi_KTXCache_h

libraries/model-networking/src/model-networking/TextureCache.cpp

@@ -58,8 +58,8 @@ static const QUrl SPECTATOR_CAMERA_FRAME_URL("resource://spectatorCameraFrame");
 static const float SKYBOX_LOAD_PRIORITY { 10.0f }; // Make sure skybox loads first
 static const float HIGH_MIPS_LOAD_PRIORITY { 9.0f }; // Make sure high mips loads after skybox but before models
 
-TextureCache::TextureCache() :
-    _ktxCache(KTX_DIRNAME, KTX_EXT) {
+TextureCache::TextureCache() {
+    _ktxCache->initialize();
     setUnusedResourceCacheSize(0);
     setObjectName("TextureCache");
 }
@@ -718,7 +718,7 @@ void NetworkTexture::handleFinishedInitialLoad() {
         gpu::TexturePointer texture = textureCache->getTextureByHash(hash);
 
         if (!texture) {
-            KTXFilePointer ktxFile = textureCache->_ktxCache.getFile(hash);
+            auto ktxFile = textureCache->_ktxCache->getFile(hash);
             if (ktxFile) {
                 texture = gpu::Texture::unserialize(ktxFile);
                 if (texture) {
@@ -742,9 +742,9 @@ void NetworkTexture::handleFinishedInitialLoad() {
             // Move ktx to file
             const char* data = reinterpret_cast<const char*>(memKtx->_storage->data());
             size_t length = memKtx->_storage->size();
-            KTXFilePointer file;
+            cache::FilePointer file;
             auto& ktxCache = textureCache->_ktxCache;
-            if (!memKtx || !(file = ktxCache.writeFile(data, KTXCache::Metadata(filename, length)))) {
+            if (!memKtx || !(file = ktxCache->writeFile(data, KTXCache::Metadata(filename, length)))) {
                 qCWarning(modelnetworking) << url << " failed to write cache file";
                 QMetaObject::invokeMethod(resource.data(), "setImage",
                     Q_ARG(gpu::TexturePointer, nullptr),
@@ -900,7 +900,7 @@ void ImageReader::read() {
 
         // If there is no live texture, check if there's an existing KTX file
         if (!texture) {
-            KTXFilePointer ktxFile = textureCache->_ktxCache.getFile(hash);
+            auto ktxFile = textureCache->_ktxCache->getFile(hash);
             if (ktxFile) {
                 texture = gpu::Texture::unserialize(ktxFile);
                 if (texture) {
@@ -950,7 +950,7 @@ void ImageReader::read() {
             const char* data = reinterpret_cast<const char*>(memKtx->_storage->data());
             size_t length = memKtx->_storage->size();
             auto& ktxCache = textureCache->_ktxCache;
-            auto file = ktxCache.writeFile(data, KTXCache::Metadata(hash, length));
+            auto file = ktxCache->writeFile(data, KTXCache::Metadata(hash, length));
             if (!file) {
                 qCWarning(modelnetworking) << _url << "file cache failed";
             } else {

libraries/model-networking/src/model-networking/TextureCache.h

@@ -189,7 +189,7 @@ private:
     static const std::string KTX_DIRNAME;
     static const std::string KTX_EXT;
 
-    KTXCache _ktxCache;
+    std::shared_ptr<cache::FileCache> _ktxCache { std::make_shared<KTXCache>(KTX_DIRNAME, KTX_EXT) };
     // Map from image hashes to texture weak pointers
     std::unordered_map<std::string, std::weak_ptr<gpu::Texture>> _texturesByHashes;
     std::mutex _texturesByHashesMutex;

libraries/shared/src/shared/FileCache.cpp

@@ -118,12 +118,18 @@ void FileCache::initialize() {
     _initialized = true;
 }
 
+std::unique_ptr<File> FileCache::createFile(Metadata&& metadata, const std::string& filepath) {
+    return std::unique_ptr<File>(new cache::File(std::move(metadata), filepath));
+}
+
 FilePointer FileCache::addFile(Metadata&& metadata, const std::string& filepath) {
-    FilePointer file(createFile(std::move(metadata), filepath).release(), std::mem_fn(&File::deleter));
+    File* rawFile = createFile(std::move(metadata), filepath).release();
+    FilePointer file(rawFile, std::bind(&File::deleter, rawFile));
     if (file) {
         _numTotalFiles += 1;
         _totalFilesSize += file->getLength();
-        file->_cache = this;
+        file->_parent = shared_from_this();
+        file->_locked = true;
         emit dirty();
 
         _files[file->getKey()] = file;
@@ -170,7 +176,7 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata, bo
     } else {
         qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str());
     }
-
+    assert(!file || (file->_locked && file->_parent.lock()));
     return file;
 }
 
@@ -192,8 +198,12 @@ FilePointer FileCache::getFile(const Key& key) {
             file->touch();
             // if it exists, it is active - remove it from the cache
             if (_unusedFiles.erase(file)) {
+                assert(!file->_locked);
+                file->_locked = true;
                 _numUnusedFiles -= 1;
                 _unusedFilesSize -= file->getLength();
+            } else {
+                assert(file->_locked);
             }
             qCDebug(file_cache, "[%s] Found %s", _dirname.c_str(), key.c_str());
             emit dirty();
@@ -203,6 +213,7 @@ FilePointer FileCache::getFile(const Key& key) {
         }
     }
 
+    assert(!file || (file->_locked && file->_parent.lock()));
     return file;
 }
 
@@ -213,7 +224,8 @@ std::string FileCache::getFilepath(const Key& key) {
 // This is a non-public function that uses the mutex because it's 
 // essentially a public function specifically to a File object
 void FileCache::addUnusedFile(const FilePointer& file) {
-    Lock lock(_mutex);
+    assert(file->_locked);
+    file->_locked = false;
     _files[file->getKey()] = file;
     _unusedFiles.insert(file);
     _numUnusedFiles += 1;
@@ -247,7 +259,7 @@ namespace cache {
 }
 
 void FileCache::eject(const FilePointer& file) {
-    file->_cache = nullptr;
+    file->_locked = false;
     const auto& length = file->getLength();
     const auto& key = file->getKey();
 
@@ -300,20 +312,34 @@ void FileCache::clear() {
     // Mark everything remaining as persisted while effectively ejecting from the cache
     for (auto& file : _unusedFiles) {
         file->_shouldPersist = true;
-        file->_cache = nullptr;
+        file->_parent.reset();
         qCDebug(file_cache, "[%s] Persisting %s", _dirname.c_str(), file->getKey().c_str());
     }
     _unusedFiles.clear();
 }
 
-void File::deleter() {
-    if (_cache) {
-        _cache->addUnusedFile(FilePointer(this, std::mem_fn(&File::deleter)));
+void FileCache::releaseFile(File* file) {
+    Lock lock(_mutex);
+    if (file->_locked) {
+        addUnusedFile(FilePointer(file, std::bind(&File::deleter, file)));
     } else {
-        deleteLater();
+        delete file;
     }
 }
 
+void File::deleter(File* file) {
+    // If the cache shut down before the file was destroyed, then we should leave the file alone (prevents crash on shutdown)
+    FileCachePointer cache = file->_parent.lock();
+    if (!cache) {
+        file->_shouldPersist = true;
+        delete file;
+        return;
+    }
+
+    // Any other operations we might do should be done inside a locked section, so we need to delegate to a FileCache member function
+    cache->releaseFile(file);
+}
+
 File::File(Metadata&& metadata, const std::string& filepath) :
     _key(std::move(metadata.key)),
     _length(metadata.length),
@@ -333,3 +359,4 @@ void File::touch() {
     utime(_filepath.c_str(), nullptr);
     _modified = std::max<int64_t>(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified);
 }
+

libraries/shared/src/shared/FileCache.h

@@ -24,12 +24,17 @@
 
 Q_DECLARE_LOGGING_CATEGORY(file_cache)
 
+class FileCacheTests;
+
 namespace cache {
 
 class File;
 using FilePointer = std::shared_ptr<File>;
+class FileCache;
+using FileCachePointer = std::shared_ptr<FileCache>;
+using FileCacheWeakPointer = std::weak_ptr<FileCache>;
 
-class FileCache : public QObject {
+class FileCache : public QObject, public std::enable_shared_from_this<FileCache> {
     Q_OBJECT
     Q_PROPERTY(size_t numTotal READ getNumTotalFiles NOTIFY dirty)
     Q_PROPERTY(size_t numCached READ getNumCachedFiles NOTIFY dirty)
@@ -40,6 +45,8 @@ class FileCache : public QObject {
     static const size_t MAX_MAX_SIZE;
     static const size_t DEFAULT_MIN_FREE_STORAGE_SPACE;
 
+    friend class ::FileCacheTests;
+
 public:
     // You can initialize the FileCache with a directory name (ex.: "temp_jpgs") that will be relative to the application local data, OR with a full path
     // The file cache will ignore any file that doesn't match the extension provided
@@ -86,14 +93,14 @@ signals:
 
 public:
     /// must be called after construction to create the cache on the fs and restore persisted files
-    void initialize();
+    virtual void initialize();
 
     // Add file to the cache and return the cache entry.  
     FilePointer writeFile(const char* data, Metadata&& metadata, bool overwrite = false);
     FilePointer getFile(const Key& key);
 
     /// create a file
-    virtual std::unique_ptr<File> createFile(Metadata&& metadata, const std::string& filepath) = 0;
+    virtual std::unique_ptr<File> createFile(Metadata&& metadata, const std::string& filepath);
 
 private:
     using Mutex = std::recursive_mutex;
@@ -108,6 +115,7 @@ private:
 
     FilePointer addFile(Metadata&& metadata, const std::string& filepath);
     void addUnusedFile(const FilePointer& file);
+    void releaseFile(File* file);
     void clean();
     void clear();
     // Remove a file from the cache
@@ -134,9 +142,7 @@ private:
     Set _unusedFiles;
 };
 
-class File : public QObject {
-    Q_OBJECT
-
+class File {
 public:
     using Key = FileCache::Key;
     using Metadata = FileCache::Metadata;
@@ -147,7 +153,7 @@ public:
 
     virtual ~File();
     /// overrides should call File::deleter to maintain caching behavior
-    virtual void deleter();
+    static void deleter(File* file);
 
 protected:
     /// when constructed, the file has already been created/written
@@ -156,14 +162,16 @@ protected:
 private:
     friend class FileCache;
     friend struct FilePointerComparator;
+    friend class ::FileCacheTests;
 
     const Key _key;
     const size_t _length;
     const std::string _filepath;
 
     void touch();
-    FileCache* _cache { nullptr };
+    FileCacheWeakPointer _parent;
     int64_t _modified { 0 };
+    bool _locked { false };
 
     bool _shouldPersist { false };
 };

tests/shared/src/FileCacheTests.cpp

@@ -10,7 +10,7 @@
 
 #include "FileCacheTests.h"
 
-#include <FileCache.h>
+#include <shared/FileCache.h>
 
 QTEST_GUILESS_MAIN(FileCacheTests)
 
@@ -24,40 +24,6 @@ static std::string getFileKey(int i) {
     return QString(QByteArray { 1, (char)i }.toHex()).toStdString();
 }
 
-class TestFile : public File {
-    using Parent = File;
-public:
-    TestFile(Metadata&& metadata, const std::string& filepath)
-        : Parent(std::move(metadata), filepath) {
-    }
-};
-
-class TestFileCache : public FileCache {
-    using Parent = FileCache;
-
-public:
-    TestFileCache(const std::string& dirname, const std::string& ext, QObject* parent = nullptr) : Parent(dirname, ext, nullptr) {
-        initialize();
-    }
-
-    std::unique_ptr<File> createFile(Metadata&& metadata, const std::string& filepath) override {
-        qCInfo(file_cache) << "Wrote KTX" << metadata.key.c_str();
-        return std::unique_ptr<File>(new TestFile(std::move(metadata), filepath));
-    }
-};
-
-using CachePointer = std::shared_ptr<TestFileCache>;
-
-// The FileCache relies on deleteLater to clear unused files, but QTest classes don't run a conventional event loop
-// so we need to call this function to force any pending deletes to occur in the File destructor
-static void forceDeletes() {
-    while (QCoreApplication::hasPendingEvents()) {
-        QCoreApplication::sendPostedEvents();
-        QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
-        QCoreApplication::processEvents();
-    }
-}
-
 size_t FileCacheTests::getCacheDirectorySize() const {
     size_t result = 0;
     QDir dir(_testDir.path());
@@ -67,8 +33,9 @@ size_t FileCacheTests::getCacheDirectorySize() const {
     return result;
 }
 
-CachePointer makeFileCache(QString& location) {
-    auto result = std::make_shared<TestFileCache>(location.toStdString(), "tmp");
+FileCachePointer makeFileCache(QString& location) {
+    auto result = std::make_shared<FileCache>(location.toStdString(), "tmp");
+    result->initialize();
     result->setMaxSize(MAX_UNUSED_SIZE);
     return result;
 }
@@ -83,26 +50,38 @@ void FileCacheTests::testUnusedFiles() {
     {
         for (int i = 0; i < 100; ++i) {
             std::string key = getFileKey(i);
-            auto file = cache->writeFile(TEST_DATA.data(), TestFileCache::Metadata(key, TEST_DATA.size()));
+            auto file = cache->writeFile(TEST_DATA.data(), FileCache::Metadata(key, TEST_DATA.size()));
+            QVERIFY(file->_locked);
             inUseFiles.push_back(file);
-            forceDeletes();
+            
             QThread::msleep(10);
         }
         QCOMPARE(cache->getNumCachedFiles(), (size_t)0);
         QCOMPARE(cache->getNumTotalFiles(), (size_t)100);
         // Release the in-use files
         inUseFiles.clear();
-        // Cache state is updated, but the directory state is unchanged, 
-        // because the file deletes are triggered by an event loop
-        QCOMPARE(cache->getNumCachedFiles(), (size_t)10);
-        QCOMPARE(cache->getNumTotalFiles(), (size_t)10);
-        QVERIFY(getCacheDirectorySize() >  MAX_UNUSED_SIZE);
-        forceDeletes();
         QCOMPARE(cache->getNumCachedFiles(), (size_t)10);
         QCOMPARE(cache->getNumTotalFiles(), (size_t)10);
         QVERIFY(getCacheDirectorySize() <= MAX_UNUSED_SIZE);
     }
 
+    // Check behavior when destroying the cache BEFORE releasing the files
+    cache = makeFileCache(_testDir.path());
+    {
+        auto directorySize = getCacheDirectorySize();
+
+        // Test files 90 to 99 are present
+        for (int i = 90; i < 100; ++i) {
+            std::string key = getFileKey(i);
+            auto file = cache->getFile(key);
+            QVERIFY(file.get());
+            inUseFiles.push_back(file);
+        }
+        cache.reset();
+        inUseFiles.clear();
+        QCOMPARE(getCacheDirectorySize(), directorySize);
+    }
+
     // Reset the cache
     cache = makeFileCache(_testDir.path());
     {
@@ -151,8 +130,6 @@ void FileCacheTests::testFreeSpacePreservation() {
     auto cache = makeFileCache(_testDir.path());
     // Setting the min free space causes it to eject the oldest files that cause the cache to exceed the minimum space
     cache->setMinFreeSize(targetFreeSpace);
-    QVERIFY(getFreeSpace() < targetFreeSpace);
-    forceDeletes();
     QCOMPARE(cache->getNumCachedFiles(), (size_t)5);
     QCOMPARE(cache->getNumTotalFiles(), (size_t)5);
     QVERIFY(getFreeSpace() >= targetFreeSpace);
@@ -176,8 +153,6 @@ void FileCacheTests::testWipe() {
     cache->wipe();
     QCOMPARE(cache->getNumCachedFiles(), (size_t)0);
     QCOMPARE(cache->getNumTotalFiles(), (size_t)0);
-    QVERIFY(getCacheDirectorySize() > 0);
-    forceDeletes();
     QCOMPARE(getCacheDirectorySize(), (size_t)0);
 }