fix locking in AvatarHashMap/AvatarManager

interface/src/avatar/AvatarManager.cpp

@@ -119,6 +119,8 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
     PerformanceTimer perfTimer("otherAvatars");
 
     // simulate avatars
+    QWriteLocker writeLock(&_hashLock);
+    
     AvatarHash::iterator avatarIterator = _avatarHash.begin();
     while (avatarIterator != _avatarHash.end()) {
         auto avatar = std::dynamic_pointer_cast<Avatar>(avatarIterator.value());
@@ -128,10 +130,9 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
             // DO NOT update or fade out uninitialized Avatars
             ++avatarIterator;
         } else if (avatar->shouldDie()) {
-            removeAvatarMotionState(avatar);
+            auto removedAvatar = avatarIterator.value();
-            _avatarFades.push_back(avatarIterator.value());
-            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
+            handleRemovedAvatar(removedAvatar);
         } else {
             avatar->startUpdate();
             avatar->simulate(deltaTime);
@@ -140,6 +141,8 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
         }
     }
     
+    writeLock.unlock();
+
     // simulate avatar fades
     simulateAvatarFades(deltaTime);
 }
@@ -173,8 +176,8 @@ AvatarSharedPointer AvatarManager::newSharedAvatar() {
 }
 
 // virtual
-AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
+AvatarSharedPointer AvatarManager::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
-    auto avatar = std::dynamic_pointer_cast<Avatar>(AvatarHashMap::addAvatar(sessionUUID, mixerWeakPointer));
+    auto avatar = std::dynamic_pointer_cast<Avatar>(AvatarHashMap::newOrExistingAvatar(sessionUUID, mixerWeakPointer));
     render::ScenePointer scene = qApp->getMain3DScene();
     render::PendingChanges pendingChanges;
     if (DependencyManager::get<SceneScriptingInterface>()->shouldRenderAvatars()) {
@@ -200,20 +203,28 @@ void AvatarManager::removeAvatarMotionState(AvatarSharedPointer avatar) {
 
 // virtual
 void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
-    AvatarHash::iterator avatarIterator = _avatarHash.find(sessionUUID);
-    if (avatarIterator != _avatarHash.end()) {
-        std::shared_ptr<Avatar> avatar = std::dynamic_pointer_cast<Avatar>(avatarIterator.value());
-        if (avatar != _myAvatar && avatar->isInitialized()) {
-            removeAvatarMotionState(avatar);
-            _avatarFades.push_back(avatarIterator.value());
     QWriteLocker locker(&_hashLock);
-            _avatarHash.erase(avatarIterator);
+    
-        }
+    auto removedAvatar = _avatarHash.take(sessionUUID);
+    if (removedAvatar) {
+        handleRemovedAvatar(removedAvatar);
     }
 }
 
+void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) {
+    qDebug() << "Removed avatar with UUID" << uuidStringWithoutCurlyBraces(removedAvatar->getSessionUUID())
+        << "from AvatarManager";
+    
+    AvatarHashMap::handleRemovedAvatar(removedAvatar);
+    
+    removeAvatarMotionState(removedAvatar);
+    _avatarFades.push_back(removedAvatar);
+}
+
 void AvatarManager::clearOtherAvatars() {
     // clear any avatars that came from an avatar-mixer
+    QWriteLocker locker(&_hashLock);
+    
     AvatarHash::iterator avatarIterator =  _avatarHash.begin();
     while (avatarIterator != _avatarHash.end()) {
         auto avatar = std::static_pointer_cast<Avatar>(avatarIterator.value());
@@ -221,10 +232,10 @@ void AvatarManager::clearOtherAvatars() {
             // don't remove myAvatar or uninitialized avatars from the list
             ++avatarIterator;
         } else {
-            removeAvatarMotionState(avatar);
+            auto removedAvatar = avatarIterator.value();
-            _avatarFades.push_back(avatarIterator.value());
-            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
+            
+            handleRemovedAvatar(removedAvatar);
         }
     }
     _myAvatar->clearLookAtTargetAvatar();
@@ -318,9 +329,11 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents
 }
 
 void AvatarManager::updateAvatarPhysicsShape(const QUuid& id) {
-    AvatarHash::iterator avatarItr = _avatarHash.find(id);
+    auto avatarData = findAvatar(id);
-    if (avatarItr != _avatarHash.end()) {
+    
-        auto avatar = std::static_pointer_cast<Avatar>(avatarItr.value());
+    if (avatarData) {
+        auto avatar = std::dynamic_pointer_cast<Avatar>(avatarData);
+        
         AvatarMotionState* motionState = avatar->getMotionState();
         if (motionState) {
             motionState->addDirtyFlags(Simulation::DIRTY_SHAPE);
@@ -363,11 +376,6 @@ AvatarSharedPointer AvatarManager::getAvatarBySessionID(const QUuid& sessionID)
     if (sessionID == _myAvatar->getSessionUUID()) {
         return std::static_pointer_cast<Avatar>(_myAvatar);
     }
-    QReadLocker locker(&_hashLock);
+    
-    auto iter = _avatarHash.find(sessionID);
+    return findAvatar(sessionID);
-    if (iter != _avatarHash.end()) {
-        return iter.value();
-    } else {
-        return AvatarSharedPointer();
-    }
 }

interface/src/avatar/AvatarManager.h

@@ -77,9 +77,11 @@ private:
     
     // virtual overrides
     virtual AvatarSharedPointer newSharedAvatar();
-    virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
+    virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
     void removeAvatarMotionState(AvatarSharedPointer avatar);
+    
     virtual void removeAvatar(const QUuid& sessionUUID);
+    virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar);
     
     QVector<AvatarSharedPointer> _avatarFades;
     std::shared_ptr<MyAvatar> _myAvatar;

libraries/avatars/src/AvatarHashMap.cpp

@@ -42,18 +42,30 @@ AvatarSharedPointer AvatarHashMap::newSharedAvatar() {
     return std::make_shared<AvatarData>();
 }
 
-AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
+AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
+    QWriteLocker locker(&_hashLock);
+    
+    auto avatar = _avatarHash.value(sessionUUID);
+    
+    if (!avatar) {
         qCDebug(avatars) << "Adding avatar with sessionUUID " << sessionUUID << "to AvatarHashMap.";
         
-    AvatarSharedPointer avatar = newSharedAvatar();
+        avatar = newSharedAvatar();
         avatar->setSessionUUID(sessionUUID);
         avatar->setOwningAvatarMixer(mixerWeakPointer);
-    QWriteLocker locker(&_hashLock);
+        
         _avatarHash.insert(sessionUUID, avatar);
         emit avatarAddedEvent(sessionUUID);
+    }
+    
     return avatar;
 }
 
+AvatarSharedPointer AvatarHashMap::findAvatar(const QUuid& sessionUUID) {
+    QReadLocker locker(&_hashLock);
+    return _avatarHash.value(sessionUUID);
+}
+
 void AvatarHashMap::processAvatarDataPacket(QSharedPointer<NLPacket> packet, SharedNodePointer sendingNode) {
 
     // enumerate over all of the avatars in this packet
@@ -66,10 +78,7 @@ void AvatarHashMap::processAvatarDataPacket(QSharedPointer<NLPacket> packet, Sha
         QByteArray byteArray = packet->readWithoutCopy(packet->bytesLeftToRead());
         
         if (sessionUUID != _lastOwnerSessionUUID) {
-            AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+            auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-            if (!avatar) {
-                avatar = addAvatar(sessionUUID, sendingNode);
-            }
             
             // have the matching (or new) avatar parse the data from the packet
             int bytesRead = avatar->parseDataFromBuffer(byteArray);
@@ -97,10 +106,8 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer<NLPacket> packet,
         identityStream >> sessionUUID >> faceMeshURL >> skeletonURL >> attachmentData >> displayName;
 
         // mesh URL for a UUID, find avatar in our list
-        AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+        auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-        if (!avatar) {
+        
-            avatar = addAvatar(sessionUUID, sendingNode);
-        }
         if (avatar->getFaceModelURL() != faceMeshURL) {
             avatar->setFaceModelURL(faceMeshURL);
         }
@@ -122,10 +129,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer<NLPacket> packet,
 void AvatarHashMap::processAvatarBillboardPacket(QSharedPointer<NLPacket> packet, SharedNodePointer sendingNode) {
     QUuid sessionUUID = QUuid::fromRfc4122(packet->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
 
-    AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+    auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-    if (!avatar) {
-        avatar = addAvatar(sessionUUID, sendingNode);
-    }
 
     QByteArray billboard = packet->read(packet->bytesLeftToRead());
     if (avatar->getBillboard() != billboard) {
@@ -137,13 +141,22 @@ void AvatarHashMap::processKillAvatar(QSharedPointer<NLPacket> packet, SharedNod
     // read the node id
     QUuid sessionUUID = QUuid::fromRfc4122(packet->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
     removeAvatar(sessionUUID);
-
 }
 
 void AvatarHashMap::removeAvatar(const QUuid& sessionUUID) {
     QWriteLocker locker(&_hashLock);
-    _avatarHash.remove(sessionUUID);
+    
-    emit avatarRemovedEvent(sessionUUID);
+    auto removedAvatar = _avatarHash.take(sessionUUID);
+    
+    if (removedAvatar) {
+        handleRemovedAvatar(removedAvatar);
+    }
+}
+
+void AvatarHashMap::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) {
+    qDebug() << "Removed avatar with UUID" << uuidStringWithoutCurlyBraces(removedAvatar->getSessionUUID())
+        << "from AvatarHashMap";
+    emit avatarRemovedEvent(removedAvatar->getSessionUUID());
 }
 
 void AvatarHashMap::sessionUUIDChanged(const QUuid& sessionUUID, const QUuid& oldUUID) {

libraries/avatars/src/AvatarHashMap.h

@@ -54,9 +54,12 @@ protected:
     AvatarHashMap();
 
     virtual AvatarSharedPointer newSharedAvatar();
-    virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
+    virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
+    virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID); // uses a QReadLocker on the hashLock
     virtual void removeAvatar(const QUuid& sessionUUID);
     
+    virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar);
+
     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 read from a different thread, you must read-lock the _hashLock. (Scripted write access is not supported).