From f070708b4a49c299967c78b7881df36bfa909cdc Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 8 Jun 2016 15:53:54 -0700 Subject: [PATCH] _avatarEntityData is accessed by more than one thread. --- interface/src/avatar/Avatar.cpp | 10 ++-- interface/src/avatar/MyAvatar.cpp | 14 ++--- libraries/avatars/src/AvatarData.cpp | 79 ++++++++++++++++++---------- libraries/avatars/src/AvatarData.h | 1 + 4 files changed, 65 insertions(+), 39 deletions(-) diff --git a/interface/src/avatar/Avatar.cpp b/interface/src/avatar/Avatar.cpp index 3e22448386..b5ac8ba77d 100644 --- a/interface/src/avatar/Avatar.cpp +++ b/interface/src/avatar/Avatar.cpp @@ -240,11 +240,13 @@ void Avatar::updateAvatarEntities() { } AvatarEntityIDs recentlyDettachedAvatarEntities = getAndClearRecentlyDetachedIDs(); - foreach (auto entityID, recentlyDettachedAvatarEntities) { - if (!_avatarEntityData.contains(entityID)) { - entityTree->deleteEntity(entityID, true, true); + _avatarEntitiesLock.withReadLock([&] { + foreach (auto entityID, recentlyDettachedAvatarEntities) { + if (!_avatarEntityData.contains(entityID)) { + entityTree->deleteEntity(entityID, true, true); + } } - } + }); }); if (success) { diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 62772c6933..324ff1534f 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -723,12 +723,14 @@ void MyAvatar::saveData() { settings.beginWriteArray("avatarEntityData"); int avatarEntityIndex = 0; - for (auto entityID : _avatarEntityData.keys()) { - settings.setArrayIndex(avatarEntityIndex); - settings.setValue("id", entityID); - settings.setValue("properties", _avatarEntityData.value(entityID)); - avatarEntityIndex++; - } + _avatarEntitiesLock.withReadLock([&] { + for (auto entityID : _avatarEntityData.keys()) { + settings.setArrayIndex(avatarEntityIndex); + settings.setValue("id", entityID); + settings.setValue("properties", _avatarEntityData.value(entityID)); + avatarEntityIndex++; + } + }); settings.endArray(); settings.setValue("displayName", _displayName); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index ff7438bb17..6d99d6ad81 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -900,7 +900,11 @@ bool AvatarData::processAvatarIdentity(const Identity& identity) { hasIdentityChanged = true; } - if (identity.avatarEntityData != _avatarEntityData) { + bool avatarEntityDataChanged = false; + _avatarEntitiesLock.withReadLock([&] { + avatarEntityDataChanged = (identity.avatarEntityData != _avatarEntityData); + }); + if (avatarEntityDataChanged) { setAvatarEntityData(identity.avatarEntityData); hasIdentityChanged = true; } @@ -914,7 +918,9 @@ QByteArray AvatarData::identityByteArray() { QUrl emptyURL(""); const QUrl& urlToSend = _skeletonModelURL.scheme() == "file" ? emptyURL : _skeletonModelURL; - identityStream << getSessionUUID() << urlToSend << _attachmentData << _displayName << _avatarEntityData; + _avatarEntitiesLock.withReadLock([&] { + identityStream << getSessionUUID() << urlToSend << _attachmentData << _displayName << _avatarEntityData; + }); return identityData; } @@ -1306,16 +1312,18 @@ QJsonObject AvatarData::toJson() const { root[JSON_AVATAR_ATTACHEMENTS] = attachmentsJson; } - if (!_avatarEntityData.empty()) { - QJsonArray avatarEntityJson; - for (auto entityID : _avatarEntityData.keys()) { - QVariantMap entityData; - entityData.insert("id", entityID); - entityData.insert("properties", _avatarEntityData.value(entityID)); - avatarEntityJson.push_back(QVariant(entityData).toJsonObject()); + _avatarEntitiesLock.withReadLock([&] { + if (!_avatarEntityData.empty()) { + QJsonArray avatarEntityJson; + for (auto entityID : _avatarEntityData.keys()) { + QVariantMap entityData; + entityData.insert("id", entityID); + entityData.insert("properties", _avatarEntityData.value(entityID)); + avatarEntityJson.push_back(QVariant(entityData).toJsonObject()); + } + root[JSON_AVATAR_ENTITIES] = avatarEntityJson; } - root[JSON_AVATAR_ENTITIES] = avatarEntityJson; - } + }); auto recordingBasis = getRecordingBasis(); bool success; @@ -1604,8 +1612,10 @@ void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& ent QMetaObject::invokeMethod(this, "updateAvatarEntity", Q_ARG(const QUuid&, entityID), Q_ARG(QByteArray, entityData)); return; } - _avatarEntityData.insert(entityID, entityData); - _avatarEntityDataLocallyEdited = true; + _avatarEntitiesLock.withWriteLock([&] { + _avatarEntityData.insert(entityID, entityData); + _avatarEntityDataLocallyEdited = true; + }); } void AvatarData::clearAvatarEntity(const QUuid& entityID) { @@ -1613,18 +1623,25 @@ void AvatarData::clearAvatarEntity(const QUuid& entityID) { QMetaObject::invokeMethod(this, "clearAvatarEntity", Q_ARG(const QUuid&, entityID)); return; } - _avatarEntityData.remove(entityID); - _avatarEntityDataLocallyEdited = true; + + _avatarEntitiesLock.withWriteLock([&] { + _avatarEntityData.remove(entityID); + _avatarEntityDataLocallyEdited = true; + }); } AvatarEntityMap AvatarData::getAvatarEntityData() const { + AvatarEntityMap result; if (QThread::currentThread() != thread()) { - AvatarEntityMap result; QMetaObject::invokeMethod(const_cast(this), "getAvatarEntityData", Qt::BlockingQueuedConnection, Q_RETURN_ARG(AvatarEntityMap, result)); return result; } - return _avatarEntityData; + + _avatarEntitiesLock.withReadLock([&] { + result = _avatarEntityData; + }); + return result; } void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { @@ -1632,29 +1649,33 @@ void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) { QMetaObject::invokeMethod(this, "setAvatarEntityData", Q_ARG(const AvatarEntityMap&, avatarEntityData)); return; } - if (_avatarEntityData != avatarEntityData) { - // keep track of entities that were attached to this avatar but no longer are - AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); + _avatarEntitiesLock.withWriteLock([&] { + if (_avatarEntityData != avatarEntityData) { + // keep track of entities that were attached to this avatar but no longer are + AvatarEntityIDs previousAvatarEntityIDs = QSet::fromList(_avatarEntityData.keys()); - _avatarEntityData = avatarEntityData; - setAvatarEntityDataChanged(true); + _avatarEntityData = avatarEntityData; + setAvatarEntityDataChanged(true); - foreach (auto entityID, previousAvatarEntityIDs) { - if (!_avatarEntityData.contains(entityID)) { - _avatarEntityDetached.insert(entityID); + foreach (auto entityID, previousAvatarEntityIDs) { + if (!_avatarEntityData.contains(entityID)) { + _avatarEntityDetached.insert(entityID); + } } } - } + }); } AvatarEntityIDs AvatarData::getAndClearRecentlyDetachedIDs() { + AvatarEntityIDs result; if (QThread::currentThread() != thread()) { - AvatarEntityIDs result; QMetaObject::invokeMethod(const_cast(this), "getRecentlyDetachedIDs", Qt::BlockingQueuedConnection, Q_RETURN_ARG(AvatarEntityIDs, result)); return result; } - AvatarEntityIDs result = _avatarEntityDetached; - _avatarEntityDetached.clear(); + _avatarEntitiesLock.withWriteLock([&] { + result = _avatarEntityDetached; + _avatarEntityDetached.clear(); + }); return result; } diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 61ee649273..2dd1079b49 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -418,6 +418,7 @@ protected: // updates about one avatar to another. glm::vec3 _globalPosition; + mutable ReadWriteLockable _avatarEntitiesLock; AvatarEntityIDs _avatarEntityDetached; // recently detached from this avatar AvatarEntityMap _avatarEntityData; bool _avatarEntityDataLocallyEdited { false };