From 3a7785d10fcd660876d81d9a0437d1821d756bef Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 18 Sep 2018 14:01:12 -0700 Subject: [PATCH 1/2] fix for deadlock in other avatar entity removal --- interface/src/avatar/AvatarManager.cpp | 40 +++++++++------- interface/src/avatar/AvatarManager.h | 5 +- libraries/avatars/src/AvatarHashMap.cpp | 62 ++++++++++++++++++------- libraries/avatars/src/AvatarHashMap.h | 2 + 4 files changed, 74 insertions(+), 35 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 1faf17ea9a..d92a5c81da 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -456,25 +456,29 @@ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar } void AvatarManager::clearOtherAvatars() { - // Remove other avatars from the world but don't actually remove them from _avatarHash - // each will either be removed on timeout or will re-added to the world on receipt of update. - const render::ScenePointer& scene = qApp->getMain3DScene(); - render::Transaction transaction; - - QReadLocker locker(&_hashLock); - AvatarHash::iterator avatarIterator = _avatarHash.begin(); - while (avatarIterator != _avatarHash.end()) { - auto avatar = std::static_pointer_cast(avatarIterator.value()); - if (avatar != _myAvatar) { - handleRemovedAvatar(avatar); - avatarIterator = _avatarHash.erase(avatarIterator); - } else { - ++avatarIterator; - } - } - assert(scene); - scene->enqueueTransaction(transaction); _myAvatar->clearLookAtTargetAvatar(); + + // setup a vector of removed avatars outside the scope of the hash lock + std::vector removedAvatars; + + { + QWriteLocker locker(&_hashLock); + + auto avatarIterator = _avatarHash.begin(); + while (avatarIterator != _avatarHash.end()) { + auto avatar = std::static_pointer_cast(avatarIterator.value()); + if (avatar != _myAvatar) { + removedAvatars.push_back(avatar); + avatarIterator = _avatarHash.erase(avatarIterator); + } else { + ++avatarIterator; + } + } + } + + for (auto& av : removedAvatars) { + handleRemovedAvatar(av); + } } void AvatarManager::deleteAllAvatars() { diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 306ba6f39b..407b3c50de 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -204,7 +204,10 @@ private: void simulateAvatarFades(float deltaTime); AvatarSharedPointer newSharedAvatar() override; - void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason) override; + + // must not be called while holding the hash lock + void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, + KillAvatarReason removalReason = KillAvatarReason::NoReason) override; QVector _avatarsToFade; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index d205a915f8..2ee51cca17 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -66,6 +66,22 @@ void AvatarReplicas::removeReplicas(const QUuid& parentID) { } } +std::vector AvatarReplicas::takeReplicas(const QUuid& parentID) { + std::vector replicas; + + auto it = _replicasMap.find(parentID); + + if (it != _replicasMap.end()) { + // take a copy of the replica shared pointers for this parent + replicas = it->second; + + // erase the replicas for this parent from our map + _replicasMap.erase(it); + } + + return replicas; +} + void AvatarReplicas::processAvatarIdentity(const QUuid& parentID, const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged) { if (_replicasMap.find(parentID) != _replicasMap.end()) { auto &replicas = _replicasMap[parentID]; @@ -386,24 +402,31 @@ void AvatarHashMap::processKillAvatar(QSharedPointer message, S } void AvatarHashMap::removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason) { - QWriteLocker locker(&_hashLock); + std::vector removedAvatars; - auto replicaIDs = _replicas.getReplicaIDs(sessionUUID); - _replicas.removeReplicas(sessionUUID); - for (auto id : replicaIDs) { - auto removedReplica = _avatarHash.take(id); - if (removedReplica) { - handleRemovedAvatar(removedReplica, removalReason); + { + QWriteLocker locker(&_hashLock); + + auto replicas = _replicas.takeReplicas(sessionUUID); + + for (auto& replica : replicas) { + auto removedReplica = _avatarHash.take(replica->getID()); + if (removedReplica) { + removedAvatars.push_back(removedReplica); + } + } + + _pendingAvatars.remove(sessionUUID); + auto removedAvatar = _avatarHash.take(sessionUUID); + + if (removedAvatar) { + removedAvatars.push_back(removedAvatar); } } - _pendingAvatars.remove(sessionUUID); - auto removedAvatar = _avatarHash.take(sessionUUID); - - if (removedAvatar) { + for (auto& removedAvatar: removedAvatars) { handleRemovedAvatar(removedAvatar, removalReason); } - } void AvatarHashMap::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { @@ -421,11 +444,18 @@ void AvatarHashMap::sessionUUIDChanged(const QUuid& sessionUUID, const QUuid& ol } void AvatarHashMap::clearOtherAvatars() { - QWriteLocker locker(&_hashLock); + QList removedAvatars; - for (auto& av : _avatarHash) { - handleRemovedAvatar(av); + { + QWriteLocker locker(&_hashLock); + + // grab a copy of the current avatars so we can call handleRemoveAvatar for them + removedAvatars = _avatarHash.values(); + + _avatarHash.clear(); } - _avatarHash.clear(); + for (auto& av : removedAvatars) { + handleRemovedAvatar(av); + } } diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index 70d7f8c04d..724dd1deac 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -49,6 +49,7 @@ public: void parseDataFromBuffer(const QUuid& parentID, const QByteArray& buffer); void processAvatarIdentity(const QUuid& parentID, const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged); void removeReplicas(const QUuid& parentID); + std::vector takeReplicas(const QUuid& parentID); void processTrait(const QUuid& parentID, AvatarTraits::TraitType traitType, QByteArray traitBinaryData); void processDeletedTraitInstance(const QUuid& parentID, AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID instanceID); void processTraitInstance(const QUuid& parentID, AvatarTraits::TraitType traitType, @@ -180,6 +181,7 @@ protected: virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID) const; // uses a QReadLocker on the hashLock virtual void removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason = KillAvatarReason::NoReason); + // must not be called while holding the hash lock virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason); AvatarHash _avatarHash; From b31837168d15d4e5090b41ff03eef443ca34c97f Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 24 Sep 2018 11:27:20 -0700 Subject: [PATCH 2/2] fix bad lock, optimize some operations, clarify comment --- interface/src/avatar/AvatarManager.cpp | 4 +++- interface/src/avatar/AvatarManager.h | 4 +++- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 3 +-- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index d92a5c81da..0fdd246d7b 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -464,6 +464,8 @@ void AvatarManager::clearOtherAvatars() { { QWriteLocker locker(&_hashLock); + removedAvatars.reserve(_avatarHash.size()); + auto avatarIterator = _avatarHash.begin(); while (avatarIterator != _avatarHash.end()) { auto avatar = std::static_pointer_cast(avatarIterator.value()); @@ -484,7 +486,7 @@ void AvatarManager::clearOtherAvatars() { void AvatarManager::deleteAllAvatars() { assert(_avatarsToChangeInPhysics.empty()); - QReadLocker locker(&_hashLock); + QWriteLocker locker(&_hashLock); AvatarHash::iterator avatarIterator = _avatarHash.begin(); while (avatarIterator != _avatarHash.end()) { auto avatar = std::static_pointer_cast(avatarIterator.value()); diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 407b3c50de..9c4287728d 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -205,7 +205,9 @@ private: AvatarSharedPointer newSharedAvatar() override; - // must not be called while holding the hash lock + // called only from the AvatarHashMap thread - cannot be called while this thread holds the + // hash lock, since handleRemovedAvatar needs a write lock on the entity tree and the entity tree + // frequently grabs a read lock on the hash to get a given avatar by ID void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason) override; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 2ee51cca17..01557e307e 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -73,7 +73,7 @@ std::vector AvatarReplicas::takeReplicas(const QUuid& paren if (it != _replicasMap.end()) { // take a copy of the replica shared pointers for this parent - replicas = it->second; + replicas.swap(it->second); // erase the replicas for this parent from our map _replicasMap.erase(it); diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index 724dd1deac..c2cb448e52 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -180,8 +180,7 @@ protected: bool& isNew); virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID) const; // uses a QReadLocker on the hashLock virtual void removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason = KillAvatarReason::NoReason); - - // must not be called while holding the hash lock + virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason); AvatarHash _avatarHash;