From 0d514ad6450f46da10d3d2a1b68becfc07f2fb12 Mon Sep 17 00:00:00 2001
From: Howard Stearns <howard@highfidelity.io>
Date: Thu, 22 Oct 2015 13:02:08 -0700
Subject: [PATCH] Thread-safe avatar list access

---
 interface/src/avatar/AvatarManager.cpp  | 8 +++++++-
 libraries/avatars/src/AvatarHashMap.cpp | 3 +++
 libraries/avatars/src/AvatarHashMap.h   | 3 +++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp
index 5f0ac435e0..20fdfdb1e9 100644
--- a/interface/src/avatar/AvatarManager.cpp
+++ b/interface/src/avatar/AvatarManager.cpp
@@ -77,7 +77,10 @@ AvatarManager::AvatarManager(QObject* parent) :
 
 void AvatarManager::init() {
     _myAvatar->init();
-    _avatarHash.insert(MY_AVATAR_KEY, _myAvatar);
+    {
+        QWriteLocker locker(&_hashLock);
+        _avatarHash.insert(MY_AVATAR_KEY, _myAvatar);
+    }
 
     connect(DependencyManager::get<SceneScriptingInterface>().data(), &SceneScriptingInterface::shouldRenderAvatarsChanged, this, &AvatarManager::updateAvatarRenderStatus, Qt::QueuedConnection);
 
@@ -127,6 +130,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
         } else if (avatar->shouldDie()) {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
         } else {
             avatar->startUpdate();
@@ -202,6 +206,7 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
         if (avatar != _myAvatar && avatar->isInitialized()) {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             _avatarHash.erase(avatarIterator);
         }
     }
@@ -218,6 +223,7 @@ void AvatarManager::clearOtherAvatars() {
         } else {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
         }
     }
diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp
index 520bb34887..4256e2650e 100644
--- a/libraries/avatars/src/AvatarHashMap.cpp
+++ b/libraries/avatars/src/AvatarHashMap.cpp
@@ -23,6 +23,7 @@ AvatarHashMap::AvatarHashMap() {
 }
 
 bool AvatarHashMap::isAvatarInRange(const glm::vec3& position, const float range) {
+    QReadLocker locker(&_hashLock);
     foreach(const AvatarSharedPointer& sharedAvatar, _avatarHash) {
         glm::vec3 avatarPosition = sharedAvatar->getPosition();
         float distance = glm::distance(avatarPosition, position);
@@ -43,6 +44,7 @@ AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWe
     AvatarSharedPointer avatar = newSharedAvatar();
     avatar->setSessionUUID(sessionUUID);
     avatar->setOwningAvatarMixer(mixerWeakPointer);
+    QWriteLocker locker(&_hashLock);
     _avatarHash.insert(sessionUUID, avatar);
 
     return avatar;
@@ -134,6 +136,7 @@ void AvatarHashMap::processKillAvatar(QSharedPointer<NLPacket> packet, SharedNod
 }
 
 void AvatarHashMap::removeAvatar(const QUuid& sessionUUID) {
+    QWriteLocker locker(&_hashLock);
     _avatarHash.remove(sessionUUID);
 }
 
diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h
index 804233b76a..142fb1cab5 100644
--- a/libraries/avatars/src/AvatarHashMap.h
+++ b/libraries/avatars/src/AvatarHashMap.h
@@ -52,6 +52,9 @@ protected:
     virtual void removeAvatar(const QUuid& sessionUUID);
 
     AvatarHash _avatarHash;
+    // "Case-based safety": Most access to the _avatarHash is on the same thread. Write access is protected by a write lock.
+    // If you access from a different thread, it is your responsibility to write- or read-lock the _hashLock.
+    QReadWriteLock _hashLock;
 
 private:
     QUuid _lastOwnerSessionUUID;