From ae8879a5581121bc264f37f2c65e1f727671ce93 Mon Sep 17 00:00:00 2001 From: HifiExperiments Date: Mon, 8 Jun 2020 16:58:21 -0700 Subject: [PATCH 1/2] possible fix for entity server crash --- assignment-client/src/entities/EntityServer.cpp | 4 +--- libraries/entities/src/EntityTree.cpp | 8 ++++++-- libraries/entities/src/EntityTree.h | 1 + 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/assignment-client/src/entities/EntityServer.cpp b/assignment-client/src/entities/EntityServer.cpp index a6ab382781..e56adfd16a 100644 --- a/assignment-client/src/entities/EntityServer.cpp +++ b/assignment-client/src/entities/EntityServer.cpp @@ -376,9 +376,7 @@ void EntityServer::nodeAdded(SharedNodePointer node) { void EntityServer::nodeKilled(SharedNodePointer node) { EntityTreePointer tree = std::static_pointer_cast(_tree); - tree->withWriteLock([&] { - tree->deleteDescendantsOfAvatar(node->getUUID()); - }); + tree->deleteDescendantsOfAvatar(node->getUUID()); tree->forgetAvatarID(node->getUUID()); OctreeServer::nodeKilled(node); } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index fedda7a42e..a22f34a874 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2213,6 +2213,7 @@ void EntityTree::fixupNeedsParentFixups() { entity->postParentFixup(); } else if (getIsServer() || _avatarIDs.contains(entity->getParentID())) { + std::lock_guard lock(_childrenOfAvatarsLock); // this is a child of an avatar, which the entity server will never have // a SpatiallyNestable object for. Add it to a list for cleanup when the avatar leaves. if (!_childrenOfAvatars.contains(entity->getParentID())) { @@ -2241,6 +2242,7 @@ void EntityTree::fixupNeedsParentFixups() { } void EntityTree::deleteDescendantsOfAvatar(QUuid avatarID) { + std::lock_guard lock(_childrenOfAvatarsLock); QHash>::const_iterator itr = _childrenOfAvatars.constFind(avatarID); if (itr != _childrenOfAvatars.end()) { if (!itr.value().empty()) { @@ -2259,8 +2261,10 @@ void EntityTree::deleteDescendantsOfAvatar(QUuid avatarID) { void EntityTree::removeFromChildrenOfAvatars(EntityItemPointer entity) { QUuid avatarID = entity->getParentID(); - if (_childrenOfAvatars.contains(avatarID)) { - _childrenOfAvatars[avatarID].remove(entity->getID()); + std::lock_guard lock(_childrenOfAvatarsLock); + auto itr = _childrenOfAvatars.find(avatarID); + if (itr != _childrenOfAvatars.end()) { + itr.value().remove(entity->getID()); } } diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 2d5119d626..85f2310edb 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -366,6 +366,7 @@ protected: // we maintain a list of avatarIDs to notice when an entity is a child of one. QSet _avatarIDs; // IDs of avatars connected to entity server + std::mutex _childrenOfAvatarsLock; QHash> _childrenOfAvatars; // which entities are children of which avatars float _maxTmpEntityLifetime { DEFAULT_MAX_TMP_ENTITY_LIFETIME }; From ef105e7de01bbf38833746b210fced708cc82638 Mon Sep 17 00:00:00 2001 From: HifiExperiments Date: Wed, 10 Jun 2020 15:56:02 -0700 Subject: [PATCH 2/2] how about this --- .../src/entities/EntityServer.cpp | 10 +++-- libraries/entities/src/EntityTree.cpp | 40 ++++++++++++++----- libraries/entities/src/EntityTree.h | 7 ++-- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/assignment-client/src/entities/EntityServer.cpp b/assignment-client/src/entities/EntityServer.cpp index e56adfd16a..4c4fcbf2dd 100644 --- a/assignment-client/src/entities/EntityServer.cpp +++ b/assignment-client/src/entities/EntityServer.cpp @@ -370,14 +370,18 @@ void EntityServer::entityFilterAdded(EntityItemID id, bool success) { void EntityServer::nodeAdded(SharedNodePointer node) { EntityTreePointer tree = std::static_pointer_cast(_tree); - tree->knowAvatarID(node->getUUID()); + if (tree) { + tree->knowAvatarID(node->getUUID()); + } OctreeServer::nodeAdded(node); } void EntityServer::nodeKilled(SharedNodePointer node) { EntityTreePointer tree = std::static_pointer_cast(_tree); - tree->deleteDescendantsOfAvatar(node->getUUID()); - tree->forgetAvatarID(node->getUUID()); + if (tree) { + tree->deleteDescendantsOfAvatar(node->getUUID()); + tree->forgetAvatarID(node->getUUID()); + } OctreeServer::nodeKilled(node); } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index a22f34a874..419f039e8c 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2212,16 +2212,24 @@ void EntityTree::fixupNeedsParentFixups() { } entity->postParentFixup(); - } else if (getIsServer() || _avatarIDs.contains(entity->getParentID())) { - std::lock_guard lock(_childrenOfAvatarsLock); - // this is a child of an avatar, which the entity server will never have - // a SpatiallyNestable object for. Add it to a list for cleanup when the avatar leaves. - if (!_childrenOfAvatars.contains(entity->getParentID())) { - _childrenOfAvatars[entity->getParentID()] = QSet(); + } else { + bool needsUpdate = getIsServer(); + if (!needsUpdate) { + std::lock_guard lock(_avatarIDsLock); + needsUpdate = _avatarIDs.contains(entity->getParentID()); + } + + if (needsUpdate) { + std::lock_guard lock(_childrenOfAvatarsLock); + // this is a child of an avatar, which the entity server will never have + // a SpatiallyNestable object for. Add it to a list for cleanup when the avatar leaves. + if (!_childrenOfAvatars.contains(entity->getParentID())) { + _childrenOfAvatars[entity->getParentID()] = QSet(); + } + _childrenOfAvatars[entity->getParentID()] += entity->getEntityItemID(); + doMove = true; + iter.remove(); // and pull it out of the list } - _childrenOfAvatars[entity->getParentID()] += entity->getEntityItemID(); - doMove = true; - iter.remove(); // and pull it out of the list } if (queryAACubeSuccess && doMove) { @@ -2241,9 +2249,19 @@ void EntityTree::fixupNeedsParentFixups() { } } -void EntityTree::deleteDescendantsOfAvatar(QUuid avatarID) { +void EntityTree::knowAvatarID(const QUuid& avatarID) { + std::lock_guard lock(_avatarIDsLock); + _avatarIDs += avatarID; +} + +void EntityTree::forgetAvatarID(const QUuid& avatarID) { + std::lock_guard lock(_avatarIDsLock); + _avatarIDs -= avatarID; +} + +void EntityTree::deleteDescendantsOfAvatar(const QUuid& avatarID) { std::lock_guard lock(_childrenOfAvatarsLock); - QHash>::const_iterator itr = _childrenOfAvatars.constFind(avatarID); + auto itr = _childrenOfAvatars.find(avatarID); if (itr != _childrenOfAvatars.end()) { if (!itr.value().empty()) { std::vector ids; diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 85f2310edb..4ee0a7dca4 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -239,9 +239,9 @@ public: Q_INVOKABLE int getJointIndex(const QUuid& entityID, const QString& name) const; Q_INVOKABLE QStringList getJointNames(const QUuid& entityID) const; - void knowAvatarID(QUuid avatarID) { _avatarIDs += avatarID; } - void forgetAvatarID(QUuid avatarID) { _avatarIDs -= avatarID; } - void deleteDescendantsOfAvatar(QUuid avatarID); + void knowAvatarID(const QUuid& avatarID); + void forgetAvatarID(const QUuid& avatarID); + void deleteDescendantsOfAvatar(const QUuid& avatarID); void removeFromChildrenOfAvatars(EntityItemPointer entity); void addToNeedsParentFixupList(EntityItemPointer entity); @@ -364,6 +364,7 @@ protected: QVector _needsParentFixup; // entites with a parentID but no (yet) known parent instance mutable QReadWriteLock _needsParentFixupLock; + std::mutex _avatarIDsLock; // we maintain a list of avatarIDs to notice when an entity is a child of one. QSet _avatarIDs; // IDs of avatars connected to entity server std::mutex _childrenOfAvatarsLock;