From 39fb94d76df9c3b06af249b2a0aa44a00c0c2589 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 8 Jun 2017 10:28:38 -0700 Subject: [PATCH 1/3] fix bug that causes hmd avatar eyes to be all the way to the side when there's not another avatar to look at --- interface/src/Application.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 5e168c6620..c00d73c9f0 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4131,7 +4131,7 @@ void Application::updateMyAvatarLookAtPosition() { lookAtSpot = transformPoint(worldHeadMat, glm::vec3(0.0f, 0.0f, TREE_SCALE)); } else { lookAtSpot = myAvatar->getHead()->getEyePosition() + - (myAvatar->getHead()->getFinalOrientationInWorldFrame() * glm::vec3(0.0f, 0.0f, -TREE_SCALE)); + (myAvatar->getHead()->getFinalOrientationInWorldFrame() * glm::vec3(0.0f, 0.0f, TREE_SCALE)); } } From a9096aa7f0b996a15fb7eaaeda45dbc25bdd2bc6 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Fri, 16 Jun 2017 14:44:43 -0700 Subject: [PATCH 2/3] Prevent deadlocks from different locking orders in FileCache --- libraries/networking/src/FileCache.cpp | 82 ++++++++++++-------------- libraries/networking/src/FileCache.h | 11 ++-- 2 files changed, 42 insertions(+), 51 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 95304e3866..f0e568fcdc 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/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, FilePointerComparator>; Queue queue; for (const auto& file : _unusedFiles) { @@ -283,7 +278,7 @@ void FileCache::clean() { } void FileCache::wipe() { - Lock unusedFilesLock(_unusedFilesMutex); + Lock lock(_mutex); while (!_unusedFiles.empty()) { eject(*_unusedFiles.begin()); } @@ -294,7 +289,6 @@ void FileCache::clear() { clean(); // Mark everything remaining as persisted - Lock unusedFilesLock(_unusedFilesMutex); for (auto& file : _unusedFiles) { file->_shouldPersist = true; file->_cache = nullptr; diff --git a/libraries/networking/src/FileCache.h b/libraries/networking/src/FileCache.h index f29d75f779..089d99273a 100644 --- a/libraries/networking/src/FileCache.h +++ b/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 _totalFilesSize { 0 }; std::atomic _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 { From a004e5fbe865c8c1a02223928b986c61cea5fba3 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Fri, 16 Jun 2017 20:04:23 -0700 Subject: [PATCH 3/3] PR feedback --- libraries/networking/src/FileCache.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index f0e568fcdc..43bab67ec7 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -285,10 +285,12 @@ void FileCache::wipe() { } void FileCache::clear() { + Lock lock(_mutex); + // Eliminate any overbudget files clean(); - // Mark everything remaining as persisted + // Mark everything remaining as persisted while effectively ejecting from the cache for (auto& file : _unusedFiles) { file->_shouldPersist = true; file->_cache = nullptr; @@ -323,4 +325,4 @@ File::~File() { void File::touch() { utime(_filepath.c_str(), nullptr); _modified = std::max(QFileInfo(_filepath.c_str()).lastRead().toMSecsSinceEpoch(), _modified); -} \ No newline at end of file +}