From aeb3f443f81da91a689bd31e9580761dd3a863b4 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 21 Jun 2017 16:14:14 -0700 Subject: [PATCH] address code review comments --- .../src/audio/AudioMixerClientData.cpp | 3 +-- assignment-client/src/avatars/AvatarMixer.cpp | 6 ++--- assignment-client/src/avatars/AvatarMixer.h | 2 +- .../src/avatars/AvatarMixerSlave.cpp | 4 +-- domain-server/src/DomainServer.cpp | 26 ++++++++++++------- libraries/avatars/src/AvatarData.cpp | 24 ++++++----------- libraries/avatars/src/AvatarData.h | 5 ++-- libraries/networking/src/LimitedNodeList.h | 5 ---- libraries/networking/src/NetworkPeer.h | 5 ++++ 9 files changed, 37 insertions(+), 43 deletions(-) diff --git a/assignment-client/src/audio/AudioMixerClientData.cpp b/assignment-client/src/audio/AudioMixerClientData.cpp index 6e11257c7f..bf19a02d9a 100644 --- a/assignment-client/src/audio/AudioMixerClientData.cpp +++ b/assignment-client/src/audio/AudioMixerClientData.cpp @@ -121,8 +121,6 @@ void AudioMixerClientData::optionallyReplicatePacket(ReceivedMessage& message, c // first, make sure that this is a packet from a node we are supposed to replicate if (node.isReplicated()) { - auto nodeList = DependencyManager::get(); - // now make sure it's a packet type that we want to replicate // first check if it is an original type that we should replicate @@ -139,6 +137,7 @@ void AudioMixerClientData::optionallyReplicatePacket(ReceivedMessage& message, c } std::unique_ptr packet; + auto nodeList = DependencyManager::get(); // enumerate the downstream audio mixers and send them the replicated version of this packet nodeList->unsafeEachNode([&](const SharedNodePointer& downstreamNode) { diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 136d5f2e8e..f1e30f4442 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -57,7 +57,7 @@ AvatarMixer::AvatarMixer(ReceivedMessage& message) : packetReceiver.registerListenerForTypes({ PacketType::ReplicatedAvatarIdentity, PacketType::ReplicatedKillAvatar - }, this, "handleReplicatedPackets"); + }, this, "handleReplicatedPacket"); packetReceiver.registerListener(PacketType::ReplicatedBulkAvatarData, this, "handleReplicatedBulkAvatarPacket"); @@ -82,7 +82,7 @@ SharedNodePointer addOrUpdateReplicatedNode(const QUuid& nodeID, const HifiSockA return replicatedNode; } -void AvatarMixer::handleReplicatedPackets(QSharedPointer message) { +void AvatarMixer::handleReplicatedPacket(QSharedPointer message) { auto nodeList = DependencyManager::get(); auto nodeID = QUuid::fromRfc4122(message->peek(NUM_BYTES_RFC4122_UUID)); @@ -96,8 +96,6 @@ void AvatarMixer::handleReplicatedPackets(QSharedPointer messag } void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer message) { - auto nodeList = DependencyManager::get(); - while (message->getBytesLeftToRead()) { // first, grab the node ID for this replicated avatar auto nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); diff --git a/assignment-client/src/avatars/AvatarMixer.h b/assignment-client/src/avatars/AvatarMixer.h index 42414dab55..58d4487652 100644 --- a/assignment-client/src/avatars/AvatarMixer.h +++ b/assignment-client/src/avatars/AvatarMixer.h @@ -46,7 +46,7 @@ private slots: void handleNodeIgnoreRequestPacket(QSharedPointer message, SharedNodePointer senderNode); void handleRadiusIgnoreRequestPacket(QSharedPointer packet, SharedNodePointer sendingNode); void handleRequestsDomainListDataPacket(QSharedPointer message, SharedNodePointer senderNode); - void handleReplicatedPackets(QSharedPointer message); + void handleReplicatedPacket(QSharedPointer message); void handleReplicatedBulkAvatarPacket(QSharedPointer message); void domainSettingsRequestComplete(); void handlePacketVersionMismatch(PacketType type, const HifiSockAddr& senderSockAddr, const QUuid& senderUUID); diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 4d5e507923..8092518454 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -67,7 +67,7 @@ void AvatarMixerSlave::processIncomingPackets(const SharedNodePointer& node) { int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) { if (destinationNode->getType() == NodeType::Agent && !destinationNode->isUpstream()) { - QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(true); + QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(); individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); identityPackets->write(individualData); @@ -81,7 +81,7 @@ int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, int AvatarMixerSlave::sendReplicatedIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) { if (destinationNode->getType() == NodeType::DownstreamAvatarMixer) { - QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(true); + QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(); individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious auto identityPacket = NLPacket::create(PacketType::ReplicatedAvatarIdentity); identityPacket->write(individualData); diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 856a568ca7..1f903ea1bc 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -2277,6 +2277,10 @@ void DomainServer::updateDownstreamNodes() { } } + + // enumerate the nodes to determine which are no longer downstream for this domain + // collect them in a vector to separately remove them with handleKillNode (since eachNode has a read lock and + // we cannot recursively take the write lock required by handleKillNode) std::vector nodesToKill; nodeList->eachNode([&](const SharedNodePointer& otherNode) { if (NodeType::isDownstream(otherNode->getType())) { @@ -2288,6 +2292,7 @@ void DomainServer::updateDownstreamNodes() { } } }); + for (auto& node : nodesToKill) { handleKillNode(node); } @@ -2296,12 +2301,11 @@ void DomainServer::updateDownstreamNodes() { void DomainServer::updateReplicatedNodes() { // Make sure we have downstream nodes in our list - // TODO Move this to a different function - _replicatedUsernames.clear(); auto settings = _settingsManager.getSettingsMap(); static const QString REPLICATED_USERS_KEY = "users"; _replicatedUsernames.clear(); + if (settings.contains(BROADCASTING_SETTINGS_KEY)) { auto replicationSettings = settings.value(BROADCASTING_SETTINGS_KEY).toMap(); if (replicationSettings.contains(REPLICATED_USERS_KEY)) { @@ -2319,9 +2323,6 @@ void DomainServer::updateReplicatedNodes() { auto shouldReplicate = shouldReplicateNode(*otherNode); auto isReplicated = otherNode->isReplicated(); if (isReplicated && !shouldReplicate) { - qDebug() << "Setting node to NOT be replicated:" << otherNode->getUUID(); - } else if (!isReplicated && shouldReplicate) { - qDebug() << "Setting node to replicated:" << otherNode->getUUID(); qDebug() << "Setting node to NOT be replicated:" << otherNode->getPermissions().getVerifiedUserName() << otherNode->getUUID(); } else if (!isReplicated && shouldReplicate) { @@ -2334,11 +2335,16 @@ void DomainServer::updateReplicatedNodes() { } bool DomainServer::shouldReplicateNode(const Node& node) { - QString verifiedUsername = node.getPermissions().getVerifiedUserName(); - // Both the verified username and usernames in _replicatedUsernames are lowercase, so - // comparisons here are case-insensitive. - auto it = find(_replicatedUsernames.cbegin(), _replicatedUsernames.cend(), verifiedUsername); - return it != _replicatedUsernames.end() && node.getType() == NodeType::Agent; + if (node.getType() == NodeType::Agent) { + QString verifiedUsername = node.getPermissions().getVerifiedUserName(); + + // Both the verified username and usernames in _replicatedUsernames are lowercase, so + // comparisons here are case-insensitive. + auto it = find(_replicatedUsernames.cbegin(), _replicatedUsernames.cend(), verifiedUsername); + return it != _replicatedUsernames.end(); + } else { + return false; + } }; void DomainServer::nodeAdded(SharedNodePointer node) { diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 6ec2b45c89..eb4a02cb62 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1496,13 +1496,13 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide udt::SequenceNumber incomingSequenceNumber(incomingSequenceNumberType); if (!_hasProcessedFirstIdentity) { - _lastIncomingSequenceNumber = incomingSequenceNumber - 1; + _lastSequenceNumber = incomingSequenceNumber - 1; _hasProcessedFirstIdentity = true; qCDebug(avatars) << "Processing first identity packet for" << avatarSessionID << "-" << (udt::SequenceNumber::Type) incomingSequenceNumber; } - if (incomingSequenceNumber > _lastIncomingSequenceNumber) { + if (incomingSequenceNumber > _lastSequenceNumber) { Identity identity; packetStream >> identity.skeletonModelURL @@ -1512,7 +1512,7 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide >> identity.avatarEntityData; // set the store identity sequence number to match the incoming identity - _lastIncomingSequenceNumber = incomingSequenceNumber; + _lastSequenceNumber = incomingSequenceNumber; if (_firstSkeletonCheck || (identity.skeletonModelURL != cannonicalSkeletonModelURL(emptyURL))) { setSkeletonModelURL(identity.skeletonModelURL); @@ -1552,34 +1552,26 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide << "identity.skeletonModelURL:" << identity.skeletonModelURL << "identity.displayName:" << identity.displayName << "identity.sessionDisplayName:" << identity.sessionDisplayName; -#endif - } else { -#ifdef WANT_DEBUG + qCDebug(avatars) << "Refusing to process identity for" << uuidStringWithoutCurlyBraces(avatarSessionID) << "since" - << (udt::SequenceNumber::Type) _lastIncomingSequenceNumber + << (udt::SequenceNumber::Type) _lastSequenceNumber << "is >=" << (udt::SequenceNumber::Type) incomingSequenceNumber; #endif } } -QByteArray AvatarData::identityByteArray(bool shouldForwardIncomingSequenceNumber) const { +QByteArray AvatarData::identityByteArray() const { QByteArray identityData; QDataStream identityStream(&identityData, QIODevice::Append); const QUrl& urlToSend = cannonicalSkeletonModelURL(emptyURL); // depends on _skeletonModelURL - // we use the boolean flag to determine if this is an identity byte array for a mixer to send to an agent - // or an agent to send to a mixer - // when mixers send identity packets to agents, they simply forward along the last incoming sequence number they received // whereas agents send a fresh outgoing sequence number when identity data has changed - udt::SequenceNumber identitySequenceNumber = - shouldForwardIncomingSequenceNumber ? _lastIncomingSequenceNumber : _lastOutgoingSequenceNumber; - _avatarEntitiesLock.withReadLock([&] { identityStream << getSessionUUID() - << (udt::SequenceNumber::Type) identitySequenceNumber + << (udt::SequenceNumber::Type) _lastSequenceNumber << urlToSend << _attachmentData << _displayName @@ -1763,7 +1755,7 @@ void AvatarData::sendIdentityPacket() { if (_identityDataChanged) { // if the identity data has changed, push the sequence number forwards - ++_lastOutgoingSequenceNumber; + ++_lastSequenceNumber; } QByteArray identityData = identityByteArray(); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 44d910b571..8d09f936b5 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -539,7 +539,7 @@ public: void processAvatarIdentity(const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged, bool& skeletonModelUrlChanged); - QByteArray identityByteArray(bool shouldForwardIncomingSequenceNumber = false) const; + QByteArray identityByteArray() const; const QUrl& getSkeletonModelURL() const { return _skeletonModelURL; } const QString& getDisplayName() const { return _displayName; } @@ -781,8 +781,7 @@ protected: float _audioAverageLoudness { 0.0f }; bool _identityDataChanged { false }; - udt::SequenceNumber _lastIncomingSequenceNumber { 0 }; - udt::SequenceNumber _lastOutgoingSequenceNumber { 0 }; + udt::SequenceNumber _lastSequenceNumber { 0 }; bool _hasProcessedFirstIdentity { false }; float _density; diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 01e0ef2dc0..ab61c71952 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -289,9 +289,6 @@ public: void sendFakedHandshakeRequestToNode(SharedNodePointer node); #endif - void setMirrorSocket(const HifiSockAddr& mirrorSocket) { _mirrorSocket = mirrorSocket; } - const HifiSockAddr& getMirrorSocket() { return _mirrorSocket; } - public slots: void reset(); void eraseAllNodes(); @@ -401,8 +398,6 @@ protected: } - HifiSockAddr _mirrorSocket; - private slots: void flagTimeForConnectionStep(ConnectionStep connectionStep, quint64 timestamp); void possiblyTimeoutSTUNAddressLookup(); diff --git a/libraries/networking/src/NetworkPeer.h b/libraries/networking/src/NetworkPeer.h index 48dd82ecb7..9842768b37 100644 --- a/libraries/networking/src/NetworkPeer.h +++ b/libraries/networking/src/NetworkPeer.h @@ -76,6 +76,11 @@ public: float getOutboundBandwidth() const; // in kbps float getInboundBandwidth() const; // in kbps + // Typically the LimitedNodeList removes nodes after they are "silent" + // meaning that we have not received any packets (including simple keepalive pings) from them for a set interval. + // The _isForcedNeverSilent flag tells the LimitedNodeList that a Node should never be killed by removeSilentNodes() + // even if its the timestamp of when it was last heard from has never been updated. + bool isForcedNeverSilent() const { return _isForcedNeverSilent; } void setIsForcedNeverSilent(bool isForcedNeverSilent) { _isForcedNeverSilent = isForcedNeverSilent; }