Merge pull request #14527 from birarda/bug/entity-traits-handler-deadlock

fix for deadlock between setAvatarEntityData and sendChangedTraitsToMixer

libraries/avatars/src/AvatarData.cpp

@@ -2833,35 +2833,47 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) {
         qCDebug(avatars) << "discard suspect AvatarEntityData with size =" << avatarEntityData.size();
         return;
     }
+
+    std::vector<QUuid> deletedEntityIDs;
+    QList<QUuid> updatedEntityIDs;
+
     _avatarEntitiesLock.withWriteLock([&] {
         if (_avatarEntityData != avatarEntityData) {
+
             // keep track of entities that were attached to this avatar but no longer are
             AvatarEntityIDs previousAvatarEntityIDs = QSet<QUuid>::fromList(_avatarEntityData.keys());
 
             _avatarEntityData = avatarEntityData;
             setAvatarEntityDataChanged(true);
 
+            deletedEntityIDs.reserve(previousAvatarEntityIDs.size());
+
             foreach (auto entityID, previousAvatarEntityIDs) {
                 if (!_avatarEntityData.contains(entityID)) {
                     _avatarEntityDetached.insert(entityID);
-
+                    deletedEntityIDs.push_back(entityID);
-                    if (_clientTraitsHandler) {
-                        // we have a client traits handler, so we flag this removed entity as deleted
-                        // so that changes are sent next frame
-                        _clientTraitsHandler->markInstancedTraitDeleted(AvatarTraits::AvatarEntity, entityID);
-                    }
                 }
             }
 
-            if (_clientTraitsHandler) {
+            updatedEntityIDs = _avatarEntityData.keys();
-                // if we have a client traits handler, flag any updated or created entities
-                // so that we send changes for them next frame
-                foreach (auto entityID, _avatarEntityData.keys()) {
-                    _clientTraitsHandler->markInstancedTraitUpdated(AvatarTraits::AvatarEntity, entityID);
-                }
-            }
         }
     });
+
+    if (_clientTraitsHandler) {
+        // we have a client traits handler
+
+        // flag removed entities as deleted so that changes are sent next frame
+        for (auto& deletedEntityID : deletedEntityIDs) {
+            _clientTraitsHandler->markInstancedTraitDeleted(AvatarTraits::AvatarEntity, deletedEntityID);
+        }
+
+        // flag any updated or created entities so that we send changes for them next frame
+        for (auto& entityID : updatedEntityIDs) {
+            _clientTraitsHandler->markInstancedTraitUpdated(AvatarTraits::AvatarEntity, entityID);
+        }
+    }
+
+
 }
 
 AvatarEntityIDs AvatarData::getAndClearRecentlyDetachedIDs() {

libraries/avatars/src/ClientTraitsHandler.cpp

@@ -66,7 +66,7 @@ void ClientTraitsHandler::resetForNewMixer() {
 }
 
 void ClientTraitsHandler::sendChangedTraitsToMixer() {
-    Lock lock(_traitLock);
+    std::unique_lock<Mutex> lock(_traitLock);
 
     if (hasChangedTraits() || _shouldPerformInitialSend) {
         // we have at least one changed trait to send
@@ -90,6 +90,14 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() {
         _traitStatuses.reset();
         _hasChangedTraits = false;
 
+        // if this was an initial send of all traits, consider it completed
+        bool initialSend = _shouldPerformInitialSend;
+        _shouldPerformInitialSend = false;
+
+        // we can release the lock here since we've taken a copy of statuses
+        // and will setup the packet using the information in the copy
+        lock.unlock();
+
         auto simpleIt = traitStatusesCopy.simpleCBegin();
         while (simpleIt != traitStatusesCopy.simpleCEnd()) {
             // because the vector contains all trait types (for access using trait type as index)
@@ -111,12 +119,12 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() {
         auto instancedIt = traitStatusesCopy.instancedCBegin();
         while (instancedIt != traitStatusesCopy.instancedCEnd()) {
             for (auto& instanceIDValuePair : instancedIt->instances) {
-                if ((_shouldPerformInitialSend && instanceIDValuePair.value != Deleted)
+                if ((initialSend && instanceIDValuePair.value != Deleted)
                     || instanceIDValuePair.value == Updated) {
                     // this is a changed trait we need to send or we haven't send out trait information yet
                     // ask the owning avatar to pack it
                     _owningAvatar->packTraitInstance(instancedIt->traitType, instanceIDValuePair.id, *traitsPacketList);
-                } else if (!_shouldPerformInitialSend && instanceIDValuePair.value == Deleted) {
+                } else if (!initialSend && instanceIDValuePair.value == Deleted) {
                     // pack delete for this trait instance
                     AvatarTraits::packInstancedTraitDelete(instancedIt->traitType, instanceIDValuePair.id,
                                                            *traitsPacketList);
@@ -127,9 +135,6 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() {
         }
 
         nodeList->sendPacketList(std::move(traitsPacketList), *avatarMixer);
-
-        // if this was an initial send of all traits, consider it completed
-        _shouldPerformInitialSend = false;
     }
 }