diff --git a/libraries/model-networking/src/model-networking/KTXCache.cpp b/libraries/model-networking/src/model-networking/KTXCache.cpp index 6443748920..edb09addb8 100644 --- a/libraries/model-networking/src/model-networking/KTXCache.cpp +++ b/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 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(file); -} - -KTXFilePointer KTXCache::getFile(const Key& key) { - return std::static_pointer_cast(FileCache::getFile(key)); -} std::unique_ptr KTXCache::createFile(Metadata&& metadata, const std::string& filepath) { qCInfo(file_cache) << "Wrote KTX" << metadata.key.c_str(); - return std::unique_ptr(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) {} - diff --git a/libraries/model-networking/src/model-networking/KTXCache.h b/libraries/model-networking/src/model-networking/KTXCache.h index a919c88bd7..079f1ac5b1 100644 --- a/libraries/model-networking/src/model-networking/KTXCache.h +++ b/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 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 diff --git a/libraries/model-networking/src/model-networking/TextureCache.cpp b/libraries/model-networking/src/model-networking/TextureCache.cpp index 06f0da1d09..674e4dbe3b 100644 --- a/libraries/model-networking/src/model-networking/TextureCache.cpp +++ b/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(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(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 { diff --git a/libraries/model-networking/src/model-networking/TextureCache.h b/libraries/model-networking/src/model-networking/TextureCache.h index 871a15a85e..43edc3593d 100644 --- a/libraries/model-networking/src/model-networking/TextureCache.h +++ b/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 _ktxCache { std::make_shared(KTX_DIRNAME, KTX_EXT) }; // Map from image hashes to texture weak pointers std::unordered_map> _texturesByHashes; std::mutex _texturesByHashesMutex; diff --git a/libraries/shared/src/shared/FileCache.cpp b/libraries/shared/src/shared/FileCache.cpp index 801c68e302..695336847e 100644 --- a/libraries/shared/src/shared/FileCache.cpp +++ b/libraries/shared/src/shared/FileCache.cpp @@ -118,12 +118,18 @@ void FileCache::initialize() { _initialized = true; } +std::unique_ptr FileCache::createFile(Metadata&& metadata, const std::string& filepath) { + return std::unique_ptr(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(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified); } + diff --git a/libraries/shared/src/shared/FileCache.h b/libraries/shared/src/shared/FileCache.h index 25210af1da..1580674ca0 100644 --- a/libraries/shared/src/shared/FileCache.h +++ b/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; +class FileCache; +using FileCachePointer = std::shared_ptr; +using FileCacheWeakPointer = std::weak_ptr; -class FileCache : public QObject { +class FileCache : public QObject, public std::enable_shared_from_this { 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 createFile(Metadata&& metadata, const std::string& filepath) = 0; + virtual std::unique_ptr 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 }; }; diff --git a/tests/shared/src/FileCacheTests.cpp b/tests/shared/src/FileCacheTests.cpp index 79fe9dee54..3f1c5e1a01 100644 --- a/tests/shared/src/FileCacheTests.cpp +++ b/tests/shared/src/FileCacheTests.cpp @@ -10,7 +10,7 @@ #include "FileCacheTests.h" -#include +#include 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 createFile(Metadata&& metadata, const std::string& filepath) override { - qCInfo(file_cache) << "Wrote KTX" << metadata.key.c_str(); - return std::unique_ptr(new TestFile(std::move(metadata), filepath)); - } -}; - -using CachePointer = std::shared_ptr; - -// 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(location.toStdString(), "tmp"); +FileCachePointer makeFileCache(QString& location) { + auto result = std::make_shared(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); }