Merge pull request #10759 from jherico/file_cache_shutdown

Fix crash when cache entry lifetime exceeds cache lifetime
This commit is contained in:
Brad Hefta-Gaub 2017-06-21 10:06:48 -07:00 committed by GitHub
commit c5845787da
7 changed files with 90 additions and 100 deletions

View file

@ -24,9 +24,10 @@ const int KTXCache::INVALID_VERSION = 0x00;
const char* KTXCache::SETTING_VERSION_NAME = "hifi.ktx.cache_version"; const char* KTXCache::SETTING_VERSION_NAME = "hifi.ktx.cache_version";
KTXCache::KTXCache(const std::string& dir, const std::string& ext) : KTXCache::KTXCache(const std::string& dir, const std::string& ext) :
FileCache(dir, ext) { FileCache(dir, ext) { }
initialize();
void KTXCache::initialize() {
FileCache::initialize();
Setting::Handle<int> cacheVersionHandle(SETTING_VERSION_NAME, INVALID_VERSION); Setting::Handle<int> cacheVersionHandle(SETTING_VERSION_NAME, INVALID_VERSION);
auto cacheVersion = cacheVersionHandle.get(); auto cacheVersion = cacheVersionHandle.get();
if (cacheVersion != CURRENT_VERSION) { 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) { std::unique_ptr<File> KTXCache::createFile(Metadata&& metadata, const std::string& filepath) {
qCInfo(file_cache) << "Wrote KTX" << metadata.key.c_str(); 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) {}

View file

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

View file

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

View file

@ -189,7 +189,7 @@ private:
static const std::string KTX_DIRNAME; static const std::string KTX_DIRNAME;
static const std::string KTX_EXT; 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 // Map from image hashes to texture weak pointers
std::unordered_map<std::string, std::weak_ptr<gpu::Texture>> _texturesByHashes; std::unordered_map<std::string, std::weak_ptr<gpu::Texture>> _texturesByHashes;
std::mutex _texturesByHashesMutex; std::mutex _texturesByHashesMutex;

View file

@ -118,12 +118,18 @@ void FileCache::initialize() {
_initialized = true; _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 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) { if (file) {
_numTotalFiles += 1; _numTotalFiles += 1;
_totalFilesSize += file->getLength(); _totalFilesSize += file->getLength();
file->_cache = this; file->_parent = shared_from_this();
file->_locked = true;
emit dirty(); emit dirty();
_files[file->getKey()] = file; _files[file->getKey()] = file;
@ -170,7 +176,7 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata, bo
} else { } else {
qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str());
} }
assert(!file || (file->_locked && file->_parent.lock()));
return file; return file;
} }
@ -192,8 +198,12 @@ FilePointer FileCache::getFile(const Key& key) {
file->touch(); file->touch();
// if it exists, it is active - remove it from the cache // if it exists, it is active - remove it from the cache
if (_unusedFiles.erase(file)) { if (_unusedFiles.erase(file)) {
assert(!file->_locked);
file->_locked = true;
_numUnusedFiles -= 1; _numUnusedFiles -= 1;
_unusedFilesSize -= file->getLength(); _unusedFilesSize -= file->getLength();
} else {
assert(file->_locked);
} }
qCDebug(file_cache, "[%s] Found %s", _dirname.c_str(), key.c_str()); qCDebug(file_cache, "[%s] Found %s", _dirname.c_str(), key.c_str());
emit dirty(); emit dirty();
@ -203,6 +213,7 @@ FilePointer FileCache::getFile(const Key& key) {
} }
} }
assert(!file || (file->_locked && file->_parent.lock()));
return file; 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 // This is a non-public function that uses the mutex because it's
// essentially a public function specifically to a File object // essentially a public function specifically to a File object
void FileCache::addUnusedFile(const FilePointer& file) { void FileCache::addUnusedFile(const FilePointer& file) {
Lock lock(_mutex); assert(file->_locked);
file->_locked = false;
_files[file->getKey()] = file; _files[file->getKey()] = file;
_unusedFiles.insert(file); _unusedFiles.insert(file);
_numUnusedFiles += 1; _numUnusedFiles += 1;
@ -247,7 +259,7 @@ namespace cache {
} }
void FileCache::eject(const FilePointer& file) { void FileCache::eject(const FilePointer& file) {
file->_cache = nullptr; file->_locked = false;
const auto& length = file->getLength(); const auto& length = file->getLength();
const auto& key = file->getKey(); const auto& key = file->getKey();
@ -300,20 +312,34 @@ void FileCache::clear() {
// Mark everything remaining as persisted while effectively ejecting from the cache // Mark everything remaining as persisted while effectively ejecting from the cache
for (auto& file : _unusedFiles) { for (auto& file : _unusedFiles) {
file->_shouldPersist = true; file->_shouldPersist = true;
file->_cache = nullptr; file->_parent.reset();
qCDebug(file_cache, "[%s] Persisting %s", _dirname.c_str(), file->getKey().c_str()); qCDebug(file_cache, "[%s] Persisting %s", _dirname.c_str(), file->getKey().c_str());
} }
_unusedFiles.clear(); _unusedFiles.clear();
} }
void File::deleter() { void FileCache::releaseFile(File* file) {
if (_cache) { Lock lock(_mutex);
_cache->addUnusedFile(FilePointer(this, std::mem_fn(&File::deleter))); if (file->_locked) {
addUnusedFile(FilePointer(file, std::bind(&File::deleter, file)));
} else { } 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) : File::File(Metadata&& metadata, const std::string& filepath) :
_key(std::move(metadata.key)), _key(std::move(metadata.key)),
_length(metadata.length), _length(metadata.length),
@ -333,3 +359,4 @@ void File::touch() {
utime(_filepath.c_str(), nullptr); utime(_filepath.c_str(), nullptr);
_modified = std::max<int64_t>(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified); _modified = std::max<int64_t>(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified);
} }

View file

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

View file

@ -10,7 +10,7 @@
#include "FileCacheTests.h" #include "FileCacheTests.h"
#include <FileCache.h> #include <shared/FileCache.h>
QTEST_GUILESS_MAIN(FileCacheTests) QTEST_GUILESS_MAIN(FileCacheTests)
@ -24,40 +24,6 @@ static std::string getFileKey(int i) {
return QString(QByteArray { 1, (char)i }.toHex()).toStdString(); 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 FileCacheTests::getCacheDirectorySize() const {
size_t result = 0; size_t result = 0;
QDir dir(_testDir.path()); QDir dir(_testDir.path());
@ -67,8 +33,9 @@ size_t FileCacheTests::getCacheDirectorySize() const {
return result; return result;
} }
CachePointer makeFileCache(QString& location) { FileCachePointer makeFileCache(QString& location) {
auto result = std::make_shared<TestFileCache>(location.toStdString(), "tmp"); auto result = std::make_shared<FileCache>(location.toStdString(), "tmp");
result->initialize();
result->setMaxSize(MAX_UNUSED_SIZE); result->setMaxSize(MAX_UNUSED_SIZE);
return result; return result;
} }
@ -83,26 +50,38 @@ void FileCacheTests::testUnusedFiles() {
{ {
for (int i = 0; i < 100; ++i) { for (int i = 0; i < 100; ++i) {
std::string key = getFileKey(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); inUseFiles.push_back(file);
forceDeletes();
QThread::msleep(10); QThread::msleep(10);
} }
QCOMPARE(cache->getNumCachedFiles(), (size_t)0); QCOMPARE(cache->getNumCachedFiles(), (size_t)0);
QCOMPARE(cache->getNumTotalFiles(), (size_t)100); QCOMPARE(cache->getNumTotalFiles(), (size_t)100);
// Release the in-use files // Release the in-use files
inUseFiles.clear(); 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->getNumCachedFiles(), (size_t)10);
QCOMPARE(cache->getNumTotalFiles(), (size_t)10); QCOMPARE(cache->getNumTotalFiles(), (size_t)10);
QVERIFY(getCacheDirectorySize() <= MAX_UNUSED_SIZE); 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 // Reset the cache
cache = makeFileCache(_testDir.path()); cache = makeFileCache(_testDir.path());
{ {
@ -151,8 +130,6 @@ void FileCacheTests::testFreeSpacePreservation() {
auto cache = makeFileCache(_testDir.path()); 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 // Setting the min free space causes it to eject the oldest files that cause the cache to exceed the minimum space
cache->setMinFreeSize(targetFreeSpace); cache->setMinFreeSize(targetFreeSpace);
QVERIFY(getFreeSpace() < targetFreeSpace);
forceDeletes();
QCOMPARE(cache->getNumCachedFiles(), (size_t)5); QCOMPARE(cache->getNumCachedFiles(), (size_t)5);
QCOMPARE(cache->getNumTotalFiles(), (size_t)5); QCOMPARE(cache->getNumTotalFiles(), (size_t)5);
QVERIFY(getFreeSpace() >= targetFreeSpace); QVERIFY(getFreeSpace() >= targetFreeSpace);
@ -176,8 +153,6 @@ void FileCacheTests::testWipe() {
cache->wipe(); cache->wipe();
QCOMPARE(cache->getNumCachedFiles(), (size_t)0); QCOMPARE(cache->getNumCachedFiles(), (size_t)0);
QCOMPARE(cache->getNumTotalFiles(), (size_t)0); QCOMPARE(cache->getNumTotalFiles(), (size_t)0);
QVERIFY(getCacheDirectorySize() > 0);
forceDeletes();
QCOMPARE(getCacheDirectorySize(), (size_t)0); QCOMPARE(getCacheDirectorySize(), (size_t)0);
} }