Merge pull request #10727 from jherico/file_cache_locking

Prevent deadlocks from different locking orders in FileCache

libraries/networking/src/FileCache.cpp

@@ -87,6 +87,12 @@ FileCache::~FileCache() {
 }
 
 void FileCache::initialize() {
+    Lock lock(_mutex);
+    if (_initialized) {
+        qCWarning(file_cache) << "File cache already initialized";
+        return;
+    }
+
     QDir dir(_dirpath.c_str());
 
     if (dir.exists()) {
@@ -120,21 +126,24 @@ FilePointer FileCache::addFile(Metadata&& metadata, const std::string& filepath)
         file->_cache = this;
         emit dirty();
 
-        Lock lock(_filesMutex);
         _files[file->getKey()] = file;
     }
     return file;
 }
 
 FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata, bool overwrite) {
-    assert(_initialized);
+    Lock lock(_mutex);
+
+    FilePointer file;
+    if (!_initialized) {
+        qCWarning(file_cache) << "File cache used before initialization";
+        return file;
+    }
 
     std::string filepath = getFilepath(metadata.key);
 
-    Lock lock(_filesMutex);
-
     // if file already exists, return it
-    FilePointer file = getFile(metadata.key);
+    file = getFile(metadata.key);
     if (file) {
         if (!overwrite) {
             qCWarning(file_cache, "[%s] Attempted to overwrite %s", _dirname.c_str(), metadata.key.c_str());
@@ -158,12 +167,15 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata, bo
     return file;
 }
 
+
 FilePointer FileCache::getFile(const Key& key) {
-    assert(_initialized);
+    Lock lock(_mutex);
 
     FilePointer file;
-
-    Lock lock(_filesMutex);
+    if (!_initialized) {
+        qCWarning(file_cache) << "File cache used before initialization";
+        return file;
+    }
 
     // check if file exists
     const auto it = _files.find(key);
@@ -172,7 +184,10 @@ FilePointer FileCache::getFile(const Key& key) {
         if (file) {
             file->touch();
             // if it exists, it is active - remove it from the cache
-            removeUnusedFile(file);
+            if (_unusedFiles.erase(file)) {
+                _numUnusedFiles -= 1;
+                _unusedFilesSize -= file->getLength();
+            }
             qCDebug(file_cache, "[%s] Found %s", _dirname.c_str(), key.c_str());
             emit dirty();
         } else {
@@ -188,31 +203,19 @@ std::string FileCache::getFilepath(const Key& key) {
     return _dirpath + DIR_SEP + key + EXT_SEP + _ext;
 }
 
+// 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(_filesMutex);
-        _files[file->getKey()] = file;
-    }
-
-    {
-        Lock lock(_unusedFilesMutex);
-        _unusedFiles.insert(file);
-        _numUnusedFiles += 1;
-        _unusedFilesSize += file->getLength();
-    }
+    Lock lock(_mutex);
+    _files[file->getKey()] = file;
+    _unusedFiles.insert(file);
+    _numUnusedFiles += 1;
+    _unusedFilesSize += file->getLength();
     clean();
 
     emit dirty();
 }
 
-void FileCache::removeUnusedFile(const FilePointer& file) {
-    Lock lock(_unusedFilesMutex);
-    if (_unusedFiles.erase(file)) {
-        _numUnusedFiles -= 1;
-        _unusedFilesSize -= file->getLength();
-    }
-}
-
 size_t FileCache::getOverbudgetAmount() const {
     size_t result = 0;
 
@@ -241,20 +244,13 @@ void FileCache::eject(const FilePointer& file) {
     const auto& length = file->getLength();
     const auto& key = file->getKey();
 
-    {
-        Lock lock(_filesMutex);
-        if (0 != _files.erase(key)) {
-            _numTotalFiles -= 1;
-            _totalFilesSize -= length;
-        }
+    if (0 != _files.erase(key)) {
+        _numTotalFiles -= 1;
+        _totalFilesSize -= length;
     }
-
-    {
-        Lock unusedLock(_unusedFilesMutex);
-        if (0 != _unusedFiles.erase(file)) {
-            _numUnusedFiles -= 1;
-            _unusedFilesSize -= length;
-        }
+    if (0 != _unusedFiles.erase(file)) {
+        _numUnusedFiles -= 1;
+        _unusedFilesSize -= length;
     }
 }
 
@@ -266,7 +262,6 @@ void FileCache::clean() {
         return;
     }
 
-    Lock unusedLock(_unusedFilesMutex);
     using Queue = std::priority_queue<FilePointer, std::vector<FilePointer>, FilePointerComparator>;
     Queue queue;
     for (const auto& file : _unusedFiles) {
@@ -283,18 +278,19 @@ void FileCache::clean() {
 }
 
 void FileCache::wipe() {
-    Lock unusedFilesLock(_unusedFilesMutex);
+    Lock lock(_mutex);
     while (!_unusedFiles.empty()) {
         eject(*_unusedFiles.begin());
     }
 }
 
 void FileCache::clear() {
+    Lock lock(_mutex);
+
     // Eliminate any overbudget files
     clean();
 
-    // Mark everything remaining as persisted
-    Lock unusedFilesLock(_unusedFilesMutex);
+    // Mark everything remaining as persisted while effectively ejecting from the cache
     for (auto& file : _unusedFiles) {
         file->_shouldPersist = true;
         file->_cache = nullptr;
@@ -329,4 +325,4 @@ File::~File() {
 void File::touch() {
     utime(_filepath.c_str(), nullptr);
     _modified = std::max<int64_t>(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified);
-}
+}

libraries/networking/src/FileCache.h

@@ -108,7 +108,6 @@ private:
 
     FilePointer addFile(Metadata&& metadata, const std::string& filepath);
     void addUnusedFile(const FilePointer& file);
-    void removeUnusedFile(const FilePointer& file);
     void clean();
     void clear();
     // Remove a file from the cache
@@ -125,16 +124,14 @@ private:
     std::atomic<size_t> _totalFilesSize { 0 };
     std::atomic<size_t> _unusedFilesSize { 0 };
 
-    std::string _ext;
-    std::string _dirname;
-    std::string _dirpath;
+    const std::string _ext;
+    const std::string _dirname;
+    const std::string _dirpath;
     bool _initialized { false };
 
+    Mutex _mutex;
     Map _files;
-    Mutex _filesMutex;
-
     Set _unusedFiles;
-    Mutex _unusedFilesMutex;
 };
 
 class File : public QObject {