From b5f165d48176a4c55c66160ce2ec0136b96f6430 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 23 Mar 2018 16:55:19 -0700 Subject: [PATCH 01/15] Store a short ID with the Node on the domain-server side --- domain-server/src/DomainGatekeeper.cpp | 30 +++++++++++++++++++++++++- domain-server/src/DomainGatekeeper.h | 14 ++++++++++++ libraries/networking/src/NetworkPeer.h | 5 +++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 7d0b538f6e..748c089b21 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -26,7 +27,7 @@ using SharedAssignmentPointer = QSharedPointer; DomainGatekeeper::DomainGatekeeper(DomainServer* server) : _server(server) { - + initLocalIDManagement(); } void DomainGatekeeper::addPendingAssignedNode(const QUuid& nodeUUID, const QUuid& assignmentUUID, @@ -525,6 +526,7 @@ SharedNodePointer DomainGatekeeper::addVerifiedNodeFromConnectRequest(const Node SharedNodePointer newNode = limitedNodeList->addOrUpdateNode(nodeID, nodeConnection.nodeType, nodeConnection.publicSockAddr, nodeConnection.localSockAddr); + newNode->setLocalID(findOrCreateLocalID(nodeID)); // So that we can send messages to this node at will - we need to activate the correct socket on this node now newNode->activateMatchingOrNewSymmetricSocket(discoveredSocket); @@ -1014,3 +1016,29 @@ void DomainGatekeeper::refreshGroupsCache() { _server->_settingsManager.debugDumpGroupsState(); #endif } + +void DomainGatekeeper::initLocalIDManagement() { + std::uniform_int_distribution sixteenBitRand; + std::random_device randomDevice; + std::default_random_engine engine {randomDevice()}; + _currentLocalID = sixteenBitRand(engine); + // Ensure increment is odd. + _idIncrement = sixteenBitRand(engine) | 1; +} + +Node::LocalID DomainGatekeeper::findOrCreateLocalID(const QUuid& uuid) { + auto existingLocalIDIt = _uuidToLocalID.find(uuid); + if (existingLocalIDIt != _uuidToLocalID.end()) { + return existingLocalIDIt->second; + } + + Node::LocalID newLocalID; + do { + newLocalID = _currentLocalID; + _currentLocalID += _idIncrement; + } while (_localIDToUUID.find(newLocalID) != _localIDToUUID.end()); + + _uuidToLocalID.emplace(uuid, newLocalID); + _localIDToUUID.emplace(newLocalID, uuid); + return newLocalID; +} diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 09db075e07..896997a0e7 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -120,6 +120,20 @@ private: void getGroupMemberships(const QString& username); // void getIsGroupMember(const QString& username, const QUuid groupID); void getDomainOwnerFriendsList(); + + // Local ID management. + void initLocalIDManagement(); + Node::LocalID findOrCreateLocalID(const QUuid& uuid); + struct UuidHash { + size_t operator()(const QUuid& uuid) const { return qHash(uuid); } + }; + using UUIDToLocalID = std::unordered_map ; + using LocalIDToUUID = std::unordered_map; + UUIDToLocalID _uuidToLocalID; + LocalIDToUUID _localIDToUUID; + + Node::LocalID _currentLocalID; + quint16 _idIncrement; }; diff --git a/libraries/networking/src/NetworkPeer.h b/libraries/networking/src/NetworkPeer.h index 9842768b37..f36db402ce 100644 --- a/libraries/networking/src/NetworkPeer.h +++ b/libraries/networking/src/NetworkPeer.h @@ -39,6 +39,10 @@ public: const QUuid& getUUID() const { return _uuid; } void setUUID(const QUuid& uuid) { _uuid = uuid; } + using LocalID = quint16; + LocalID getLocalID() const { return _localID; } + void setLocalID(LocalID localID) { _localID = localID; } + void softReset(); void reset(); @@ -99,6 +103,7 @@ protected: void setActiveSocket(HifiSockAddr* discoveredSocket); QUuid _uuid; + LocalID _localID { 0 }; HifiSockAddr _publicSocket; HifiSockAddr _localSocket; From 4ec77e3af6f7752564e0cf8dc23d895e1079cc67 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Mon, 26 Mar 2018 14:57:59 -0700 Subject: [PATCH 02/15] WIP - assign short IDs to nodes and distribute them. --- domain-server/src/DomainGatekeeper.h | 3 ++- domain-server/src/DomainServer.cpp | 5 +++++ libraries/networking/src/LimitedNodeList.cpp | 9 +++++++++ libraries/networking/src/LimitedNodeList.h | 3 +++ libraries/networking/src/Node.cpp | 4 +++- libraries/networking/src/NodeList.cpp | 17 ++++++++++++----- 6 files changed, 34 insertions(+), 7 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 896997a0e7..afb848f271 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -41,6 +41,8 @@ public: void removeICEPeer(const QUuid& peerUUID) { _icePeers.remove(peerUUID); } + Node::LocalID findOrCreateLocalID(const QUuid& uuid); + static void sendProtocolMismatchConnectionDenial(const HifiSockAddr& senderSockAddr); public slots: void processConnectRequestPacket(QSharedPointer message); @@ -123,7 +125,6 @@ private: // Local ID management. void initLocalIDManagement(); - Node::LocalID findOrCreateLocalID(const QUuid& uuid); struct UuidHash { size_t operator()(const QUuid& uuid) const { return qHash(uuid); } }; diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 197ac7eac2..5ae2f8514c 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -691,6 +691,10 @@ void DomainServer::setupNodeListAndAssignments() { } } + // Create our own short session ID. + Node::LocalID serverSessionLocalID = _gatekeeper.findOrCreateLocalID(nodeList->getSessionUUID()); + nodeList->setSessionLocalID(serverSessionLocalID); + if (isMetaverseDomain) { // see if we think we're a temp domain (we have an API key) or a full domain const auto& temporaryDomainKey = DependencyManager::get()->getTemporaryDomainKey(getID()); @@ -1165,6 +1169,7 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif extendedHeaderStream << limitedNodeList->getSessionUUID(); extendedHeaderStream << node->getUUID(); + extendedHeaderStream << node->getLocalID(); extendedHeaderStream << node->getPermissions(); auto domainListPackets = NLPacketList::create(PacketType::DomainList, extendedHeader); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 0803e380f2..0b2fb9475d 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -131,6 +131,15 @@ void LimitedNodeList::setSessionUUID(const QUuid& sessionUUID) { } } +Node::LocalID LimitedNodeList::getSessionLocalID() const { + return _sessionLocalID; +} + +void LimitedNodeList::setSessionLocalID(Node::LocalID sessionLocalID) { + QWriteLocker lock { &_sessionUUIDLock }; // Necessary? + _sessionLocalID = sessionLocalID; +} + void LimitedNodeList::setPermissions(const NodePermissions& newPermissions) { NodePermissions originalPermissions = _permissions; diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 7165b3dd63..c725e8abb7 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -106,6 +106,8 @@ public: Q_ENUM(ConnectionStep); QUuid getSessionUUID() const; void setSessionUUID(const QUuid& sessionUUID); + Node::LocalID getSessionLocalID() const; + void setSessionLocalID(Node::LocalID localID); void setPermissions(const NodePermissions& newPermissions); bool isAllowedEditor() const { return _permissions.can(NodePermissions::Permission::canAdjustLocks); } @@ -427,6 +429,7 @@ private slots: private: mutable QReadWriteLock _sessionUUIDLock; QUuid _sessionUUID; + Node::LocalID _sessionLocalID { 0 }; }; #endif // hifi_LimitedNodeList_h diff --git a/libraries/networking/src/Node.cpp b/libraries/networking/src/Node.cpp index bd895c8ef1..73b7c44e7e 100644 --- a/libraries/networking/src/Node.cpp +++ b/libraries/networking/src/Node.cpp @@ -168,6 +168,7 @@ QDataStream& operator<<(QDataStream& out, const Node& node) { out << node._localSocket; out << node._permissions; out << node._isReplicated; + out << node._localID; return out; } @@ -178,6 +179,7 @@ QDataStream& operator>>(QDataStream& in, Node& node) { in >> node._localSocket; in >> node._permissions; in >> node._isReplicated; + in >> node._localID; return in; } @@ -188,7 +190,7 @@ QDebug operator<<(QDebug debug, const Node& node) { } else { debug.nospace() << " (" << node.getType() << ")"; } - debug << " " << node.getUUID().toString().toLocal8Bit().constData() << " "; + debug << " " << node.getUUID().toString().toLocal8Bit().constData() << "(" << node.getLocalID() << ") "; debug.nospace() << node.getPublicSocket() << "/" << node.getLocalSocket(); return debug.nospace(); } diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index cb0d2e4cd5..752d420a4b 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -594,9 +594,13 @@ void NodeList::processDomainServerList(QSharedPointer message) return; } - // pull our owner UUID from the packet, it's always the first thing + // pull our owner (ie. session) UUID from the packet, it's always the first thing + // The short (16 bit) ID comes next. QUuid newUUID; + Node::LocalID newLocalID; packetStream >> newUUID; + packetStream >> newLocalID; + setSessionLocalID(newLocalID); setSessionUUID(newUUID); // if this was the first domain-server list from this domain, we've now connected @@ -638,12 +642,14 @@ void NodeList::processDomainServerRemovedNode(QSharedPointer me void NodeList::parseNodeFromPacketStream(QDataStream& packetStream) { // setup variables to read into from QDataStream qint8 nodeType; - QUuid nodeUUID, connectionUUID; + QUuid nodeUUID, connectionSecretUUID; HifiSockAddr nodePublicSocket, nodeLocalSocket; NodePermissions permissions; bool isReplicated; + Node::LocalID sessionLocalID; - packetStream >> nodeType >> nodeUUID >> nodePublicSocket >> nodeLocalSocket >> permissions >> isReplicated; + packetStream >> nodeType >> nodeUUID >> nodePublicSocket >> nodeLocalSocket >> permissions + >> isReplicated >> sessionLocalID; // if the public socket address is 0 then it's reachable at the same IP // as the domain server @@ -651,10 +657,11 @@ void NodeList::parseNodeFromPacketStream(QDataStream& packetStream) { nodePublicSocket.setAddress(_domainHandler.getIP()); } - packetStream >> connectionUUID; + packetStream >> connectionSecretUUID; SharedNodePointer node = addOrUpdateNode(nodeUUID, nodeType, nodePublicSocket, - nodeLocalSocket, isReplicated, false, connectionUUID, permissions); + nodeLocalSocket, isReplicated, false, connectionSecretUUID, permissions); + node->setLocalID(sessionLocalID); // nodes that are downstream or upstream of our own type are kept alive when we hear about them from the domain server // and always have their public socket as their active socket From d3464378b71e4a98a60cbaddda2be933008d7202 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 27 Mar 2018 13:46:58 -0700 Subject: [PATCH 03/15] Short local source IDs - checkpoint DS assigns 16-bit IDs as well as UUIDs; ACs track mappings; nodes use short IDs in packets. Initial setup works; then fails prob. due to DS UUID. --- assignment-client/src/assets/AssetServer.cpp | 2 +- assignment-client/src/audio/AudioMixer.cpp | 8 +++- assignment-client/src/avatars/AvatarMixer.cpp | 10 +++-- domain-server/src/DomainGatekeeper.cpp | 5 ++- domain-server/src/DomainServer.cpp | 20 +++++----- libraries/audio/src/InboundAudioStream.cpp | 6 +-- libraries/networking/src/LimitedNodeList.cpp | 39 ++++++++++++++----- libraries/networking/src/LimitedNodeList.h | 5 ++- libraries/networking/src/NLPacket.cpp | 18 +++++---- libraries/networking/src/NLPacket.h | 12 +++--- libraries/networking/src/NLPacketList.h | 4 +- libraries/networking/src/NodeList.cpp | 5 +-- libraries/networking/src/PacketReceiver.cpp | 5 +-- libraries/networking/src/ReceivedMessage.cpp | 2 +- libraries/networking/src/ReceivedMessage.h | 6 +-- libraries/octree/src/OctreeProcessor.cpp | 2 +- 16 files changed, 91 insertions(+), 58 deletions(-) diff --git a/assignment-client/src/assets/AssetServer.cpp b/assignment-client/src/assets/AssetServer.cpp index 87827a27d9..9d474c8c24 100644 --- a/assignment-client/src/assets/AssetServer.cpp +++ b/assignment-client/src/assets/AssetServer.cpp @@ -842,7 +842,7 @@ void AssetServer::handleAssetUpload(QSharedPointer message, Sha if (canWriteToAssetServer) { - qCDebug(asset_server) << "Starting an UploadAssetTask for upload from" << uuidStringWithoutCurlyBraces(message->getSourceID()); + qCDebug(asset_server) << "Starting an UploadAssetTask for upload from" << message->getSourceID(); auto task = new UploadAssetTask(message, senderNode, _filesDirectory, _filesizeLimit); _transferTaskPool.start(task); diff --git a/assignment-client/src/audio/AudioMixer.cpp b/assignment-client/src/audio/AudioMixer.cpp index 8af4eec934..3b79eab06e 100644 --- a/assignment-client/src/audio/AudioMixer.cpp +++ b/assignment-client/src/audio/AudioMixer.cpp @@ -118,7 +118,11 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess // make sure we have a replicated node for the original sender of the packet auto nodeList = DependencyManager::get(); - QUuid nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); + QUuid nodeID; + SharedNodePointer sourceNode = nodeList->nodeWithLocalID(message->getSourceID()); + if (sourceNode) { + nodeID = sourceNode->getUUID(); + } auto replicatedNode = nodeList->addOrUpdateNode(nodeID, NodeType::Agent, message->getSenderSockAddr(), message->getSenderSockAddr(), @@ -136,7 +140,7 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess auto replicatedMessage = QSharedPointer::create(audioData, rewrittenType, versionForPacketType(rewrittenType), - message->getSenderSockAddr(), nodeID); + message->getSenderSockAddr(), message->getSourceID()); getOrCreateClientData(replicatedNode.data())->queuePacket(replicatedMessage, replicatedNode); } diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 929941c05c..b0f1420472 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -112,8 +112,12 @@ void AvatarMixer::handleReplicatedPacket(QSharedPointer message void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer message) { while (message->getBytesLeftToRead()) { // first, grab the node ID for this replicated avatar - auto nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); - + QUuid nodeID; + auto nodeList = DependencyManager::get(); + SharedNodePointer sourceNode = nodeList->nodeWithLocalID(message->getSourceID()); + if (sourceNode) { + nodeID = sourceNode->getUUID(); + } // make sure we have an upstream replicated node that matches auto replicatedNode = addOrUpdateReplicatedNode(nodeID, message->getSenderSockAddr()); @@ -127,7 +131,7 @@ void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer::create(avatarByteArray, PacketType::AvatarData, versionForPacketType(PacketType::AvatarData), - message->getSenderSockAddr(), nodeID); + message->getSenderSockAddr(), message->getSourceID()); // queue up the replicated avatar data with the client data for the replicated node auto start = usecTimestampNow(); diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 748c089b21..917d8a01c0 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -524,9 +524,10 @@ SharedNodePointer DomainGatekeeper::addVerifiedNodeFromConnectRequest(const Node auto limitedNodeList = DependencyManager::get(); + Node::LocalID newLocalID = findOrCreateLocalID(nodeID); SharedNodePointer newNode = limitedNodeList->addOrUpdateNode(nodeID, nodeConnection.nodeType, - nodeConnection.publicSockAddr, nodeConnection.localSockAddr); - newNode->setLocalID(findOrCreateLocalID(nodeID)); + nodeConnection.publicSockAddr, nodeConnection.localSockAddr, + newLocalID); // So that we can send messages to this node at will - we need to activate the correct socket on this node now newNode->activateMatchingOrNewSymmetricSocket(discoveredSocket); diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 5ae2f8514c..fee102d66a 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -593,8 +593,9 @@ bool DomainServer::isPacketVerified(const udt::Packet& packet) { if (!PacketTypeEnum::getNonSourcedPackets().contains(headerType)) { // this is a sourced packet - first check if we have a node that matches - QUuid sourceID = NLPacket::sourceIDInHeader(packet); - SharedNodePointer sourceNode = nodeList->nodeWithUUID(sourceID); + //QUuid sourceID = NLPacket::sourceIDInHeader(packet); + Node::LocalID localSourceID = NLPacket::sourceIDInHeader(packet); + SharedNodePointer sourceNode = nodeList->nodeWithLocalID(localSourceID); if (sourceNode) { // unverified DS packets (due to a lack of connection secret between DS + node) @@ -616,17 +617,17 @@ bool DomainServer::isPacketVerified(const udt::Packet& packet) { = LogHandler::getInstance().addRepeatedMessageRegex(UNKNOWN_REGEX); qDebug() << "Packet of type" << headerType - << "received from unmatched IP for UUID" << uuidStringWithoutCurlyBraces(sourceID); + << "received from unmatched IP for UUID" << uuidStringWithoutCurlyBraces(sourceNode->getUUID()); return false; } } else { - static const QString UNKNOWN_REGEX = "Packet of type \\d+ \\([\\sa-zA-Z:]+\\) received from unknown node with UUID"; + static const QString UNKNOWN_REGEX = "Packet of type \\d+ \\([\\sa-zA-Z:]+\\) received from unknown node with Local ID"; static QString repeatedMessage = LogHandler::getInstance().addRepeatedMessageRegex(UNKNOWN_REGEX); qDebug() << "Packet of type" << headerType - << "received from unknown node with UUID" << uuidStringWithoutCurlyBraces(sourceID); + << "received from unknown node with Local ID" << localSourceID; return false; } @@ -3203,13 +3204,12 @@ void DomainServer::processNodeDisconnectRequestPacket(QSharedPointer(); - const QUuid& nodeUUID = message->getSourceID(); - - qDebug() << "Received a disconnect request from node with UUID" << nodeUUID; + auto localID = message->getSourceID(); + qDebug() << "Received a disconnect request from node with local ID" << localID; // we want to check what type this node was before going to kill it so that we can avoid sending the RemovedNode // packet to nodes that don't care about this type - auto nodeToKill = limitedNodeList->nodeWithUUID(nodeUUID); + auto nodeToKill = limitedNodeList->nodeWithLocalID(localID); if (nodeToKill) { handleKillNode(nodeToKill); @@ -3477,7 +3477,7 @@ void DomainServer::handleDomainContentReplacementFromURLRequest(QSharedPointer message) { - auto node = DependencyManager::get()->nodeWithUUID(message->getSourceID()); + auto node = DependencyManager::get()->nodeWithLocalID(message->getSourceID()); if (node->getCanReplaceContent()) { handleOctreeFileReplacement(message->readAll()); } diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 72acc7fcf6..983b5e1cb8 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -120,8 +120,8 @@ int InboundAudioStream::parseData(ReceivedMessage& message) { // parse sequence number and track it quint16 sequence; message.readPrimitive(&sequence); - SequenceNumberStats::ArrivalInfo arrivalInfo = _incomingSequenceNumberStats.sequenceNumberReceived(sequence, - message.getSourceID()); + SequenceNumberStats::ArrivalInfo arrivalInfo = _incomingSequenceNumberStats.sequenceNumberReceived(sequence, QUuid() // TBD + /*message.getSourceID()*/); QString codecInPacket = message.readString(); packetReceivedUpdateTimingStats(); @@ -186,7 +186,7 @@ int InboundAudioStream::parseData(ReceivedMessage& message) { _mismatchedAudioCodecCount = 0; // inform others of the mismatch - auto sendingNode = DependencyManager::get()->nodeWithUUID(message.getSourceID()); + auto sendingNode = DependencyManager::get()->nodeWithLocalID(message.getSourceID()); if (sendingNode) { emit mismatchedAudioCodec(sendingNode, _selectedCodecName, codecInPacket); qDebug(audio) << "Codec mismatch threshold exceeded, SelectedAudioFormat(" << _selectedCodecName << " ) sent"; diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 0b2fb9475d..77daaa84ea 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -238,13 +238,16 @@ bool LimitedNodeList::packetVersionMatch(const udt::Packet& packet) { senderString = QString("%1:%2").arg(senderSockAddr.getAddress().toString()).arg(senderSockAddr.getPort()); } } else { - sourceID = NLPacket::sourceIDInHeader(packet); + SharedNodePointer sourceNode = nodeWithLocalID(NLPacket::sourceIDInHeader(packet)); + if (sourceNode) { + sourceID = sourceNode->getUUID(); - hasBeenOutput = sourcedVersionDebugSuppressMap.contains(sourceID, headerType); + hasBeenOutput = sourcedVersionDebugSuppressMap.contains(sourceID, headerType); - if (!hasBeenOutput) { - sourcedVersionDebugSuppressMap.insert(sourceID, headerType); - senderString = uuidStringWithoutCurlyBraces(sourceID.toString()); + if (!hasBeenOutput) { + sourcedVersionDebugSuppressMap.insert(sourceID, headerType); + senderString = uuidStringWithoutCurlyBraces(sourceID.toString()); + } } } @@ -302,14 +305,17 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe return true; } } else { - QUuid sourceID = NLPacket::sourceIDInHeader(packet); // check if we were passed a sourceNode hint or if we need to look it up if (!sourceNode) { // figure out which node this is from - SharedNodePointer matchingNode = nodeWithUUID(sourceID); + NLPacket::LocalID sourceLocalID = NLPacket::sourceIDInHeader(packet); + + SharedNodePointer matchingNode = nodeWithLocalID(sourceLocalID); sourceNode = matchingNode.data(); } + + QUuid sourceID = sourceNode->getUUID(); if (!sourceNode && sourceID == getDomainUUID() && @@ -374,7 +380,7 @@ void LimitedNodeList::collectPacketStats(const NLPacket& packet) { void LimitedNodeList::fillPacketHeader(const NLPacket& packet, const QUuid& connectionSecret) { if (!PacketTypeEnum::getNonSourcedPackets().contains(packet.getType())) { - packet.writeSourceID(getSessionUUID()); + packet.writeSourceID(getSessionLocalID()); } if (!connectionSecret.isNull() @@ -557,6 +563,16 @@ SharedNodePointer LimitedNodeList::nodeWithUUID(const QUuid& nodeUUID) { return it == _nodeHash.cend() ? SharedNodePointer() : it->second; } +SharedNodePointer LimitedNodeList::nodeWithLocalID(Node::LocalID localID) const { + QReadLocker readLocker(&_nodeMutex); + + LocalIDMapping::const_iterator idIter = _localIDMap.find(localID); + if (idIter == _localIDMap.cend()) { + qCDebug(networking) << "No such Node with local ID " << localID; + } + return idIter == _localIDMap.cend() ? nullptr : idIter->second; +} + void LimitedNodeList::eraseAllNodes() { QSet killedNodes; @@ -565,6 +581,8 @@ void LimitedNodeList::eraseAllNodes() { // and then remove them from the hash QWriteLocker writeLocker(&_nodeMutex); + _localIDMap.erase(_localIDMap.begin(), _localIDMap.end()); + if (_nodeHash.size() > 0) { qCDebug(networking) << "LimitedNodeList::eraseAllNodes() removing all nodes from NodeList."; @@ -630,7 +648,7 @@ void LimitedNodeList::handleNodeKill(const SharedNodePointer& node) { SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t nodeType, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, - bool isReplicated, bool isUpstream, + Node::LocalID localID, bool isReplicated, bool isUpstream, const QUuid& connectionSecret, const NodePermissions& permissions) { QReadLocker readLocker(&_nodeMutex); NodeHash::const_iterator it = _nodeHash.find(uuid); @@ -644,6 +662,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t matchingNode->setConnectionSecret(connectionSecret); matchingNode->setIsReplicated(isReplicated); matchingNode->setIsUpstream(isUpstream || NodeType::isUpstream(nodeType)); + matchingNode->setLocalID(localID); return matchingNode; } else { @@ -653,6 +672,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t newNode->setIsUpstream(isUpstream || NodeType::isUpstream(nodeType)); newNode->setConnectionSecret(connectionSecret); newNode->setPermissions(permissions); + newNode->setLocalID(localID); // move the newly constructed node to the LNL thread newNode->moveToThread(thread()); @@ -693,6 +713,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t #else _nodeHash.emplace(newNode->getUUID(), newNodePointer); #endif + _localIDMap.emplace(localID, newNodePointer); readLocker.unlock(); qCDebug(networking) << "Added" << *newNode; diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index c725e8abb7..51c8831e2b 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -157,10 +157,11 @@ public: size_t size() const { QReadLocker readLock(&_nodeMutex); return _nodeHash.size(); } SharedNodePointer nodeWithUUID(const QUuid& nodeUUID); + SharedNodePointer nodeWithLocalID(Node::LocalID localID) const; SharedNodePointer addOrUpdateNode(const QUuid& uuid, NodeType_t nodeType, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, - bool isReplicated = false, bool isUpstream = false, + Node::LocalID localID = 0, bool isReplicated = false, bool isUpstream = false, const QUuid& connectionSecret = QUuid(), const NodePermissions& permissions = DEFAULT_AGENT_PERMISSIONS); @@ -429,6 +430,8 @@ private slots: private: mutable QReadWriteLock _sessionUUIDLock; QUuid _sessionUUID; + using LocalIDMapping = std::unordered_map; + LocalIDMapping _localIDMap; Node::LocalID _sessionLocalID { 0 }; }; diff --git a/libraries/networking/src/NLPacket.cpp b/libraries/networking/src/NLPacket.cpp index 5c5077691b..66e74238aa 100644 --- a/libraries/networking/src/NLPacket.cpp +++ b/libraries/networking/src/NLPacket.cpp @@ -14,7 +14,7 @@ int NLPacket::localHeaderSize(PacketType type) { bool nonSourced = PacketTypeEnum::getNonSourcedPackets().contains(type); bool nonVerified = PacketTypeEnum::getNonVerifiedPackets().contains(type); - qint64 optionalSize = (nonSourced ? 0 : NUM_BYTES_RFC4122_UUID) + ((nonSourced || nonVerified) ? 0 : NUM_BYTES_MD5_HASH); + qint64 optionalSize = (nonSourced ? 0 : NUM_BYTES_LOCALID) + ((nonSourced || nonVerified) ? 0 : NUM_BYTES_MD5_HASH); return sizeof(PacketType) + sizeof(PacketVersion) + optionalSize; } int NLPacket::totalHeaderSize(PacketType type, bool isPartOfMessage) { @@ -139,13 +139,14 @@ PacketVersion NLPacket::versionInHeader(const udt::Packet& packet) { return *reinterpret_cast(packet.getData() + headerOffset + sizeof(PacketType)); } -QUuid NLPacket::sourceIDInHeader(const udt::Packet& packet) { +NLPacket::LocalID NLPacket::sourceIDInHeader(const udt::Packet& packet) { int offset = Packet::totalHeaderSize(packet.isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion); - return QUuid::fromRfc4122(QByteArray::fromRawData(packet.getData() + offset, NUM_BYTES_RFC4122_UUID)); + return *reinterpret_cast(packet.getData() + offset); } QByteArray NLPacket::verificationHashInHeader(const udt::Packet& packet) { - int offset = Packet::totalHeaderSize(packet.isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion) + NUM_BYTES_RFC4122_UUID; + int offset = Packet::totalHeaderSize(packet.isPartOfMessage()) + sizeof(PacketType) + + sizeof(PacketVersion) + NUM_BYTES_LOCALID; return QByteArray(packet.getData() + offset, NUM_BYTES_MD5_HASH); } @@ -153,7 +154,7 @@ QByteArray NLPacket::hashForPacketAndSecret(const udt::Packet& packet, const QUu QCryptographicHash hash(QCryptographicHash::Md5); int offset = Packet::totalHeaderSize(packet.isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion) - + NUM_BYTES_RFC4122_UUID + NUM_BYTES_MD5_HASH; + + NUM_BYTES_LOCALID + NUM_BYTES_MD5_HASH; // add the packet payload and the connection UUID hash.addData(packet.getData() + offset, packet.getDataSize() - offset); @@ -203,11 +204,12 @@ void NLPacket::readSourceID() { } } -void NLPacket::writeSourceID(const QUuid& sourceID) const { +void NLPacket::writeSourceID(LocalID sourceID) const { Q_ASSERT(!PacketTypeEnum::getNonSourcedPackets().contains(_type)); auto offset = Packet::totalHeaderSize(isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion); - memcpy(_packet.get() + offset, sourceID.toRfc4122().constData(), NUM_BYTES_RFC4122_UUID); + + memcpy(_packet.get() + offset, &sourceID, sizeof(sourceID)); _sourceID = sourceID; } @@ -217,7 +219,7 @@ void NLPacket::writeVerificationHashGivenSecret(const QUuid& connectionSecret) c !PacketTypeEnum::getNonVerifiedPackets().contains(_type)); auto offset = Packet::totalHeaderSize(isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion) - + NUM_BYTES_RFC4122_UUID; + + NUM_BYTES_LOCALID; QByteArray verificationHash = hashForPacketAndSecret(*this, connectionSecret); memcpy(_packet.get() + offset, verificationHash.data(), verificationHash.size()); diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index f49cc47645..a4690a376c 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -43,10 +43,12 @@ public: // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | // NLPacket Header Format + using LocalID = qint16; + static const int NUM_BYTES_LOCALID = sizeof(LocalID); // this is used by the Octree classes - must be known at compile time static const int MAX_PACKET_HEADER_SIZE = sizeof(udt::Packet::SequenceNumberAndBitField) + sizeof(udt::Packet::MessageNumberAndBitField) + - sizeof(PacketType) + sizeof(PacketVersion) + NUM_BYTES_RFC4122_UUID + NUM_BYTES_MD5_HASH; + sizeof(PacketType) + sizeof(PacketVersion) + NUM_BYTES_LOCALID + NUM_BYTES_MD5_HASH; static std::unique_ptr create(PacketType type, qint64 size = -1, bool isReliable = false, bool isPartOfMessage = false, PacketVersion version = 0); @@ -69,7 +71,7 @@ public: static PacketType typeInHeader(const udt::Packet& packet); static PacketVersion versionInHeader(const udt::Packet& packet); - static QUuid sourceIDInHeader(const udt::Packet& packet); + static LocalID sourceIDInHeader(const udt::Packet& packet); static QByteArray verificationHashInHeader(const udt::Packet& packet); static QByteArray hashForPacketAndSecret(const udt::Packet& packet, const QUuid& connectionSecret); @@ -79,9 +81,9 @@ public: PacketVersion getVersion() const { return _version; } void setVersion(PacketVersion version); - const QUuid& getSourceID() const { return _sourceID; } + LocalID getSourceID() const { return _sourceID; } - void writeSourceID(const QUuid& sourceID) const; + void writeSourceID(qint16 sourceID) const; void writeVerificationHashGivenSecret(const QUuid& connectionSecret) const; protected: @@ -106,7 +108,7 @@ protected: PacketType _type; PacketVersion _version; - mutable QUuid _sourceID; + mutable LocalID _sourceID; }; #endif // hifi_NLPacket_h diff --git a/libraries/networking/src/NLPacketList.h b/libraries/networking/src/NLPacketList.h index 910d39f71b..9c50033ca7 100644 --- a/libraries/networking/src/NLPacketList.h +++ b/libraries/networking/src/NLPacketList.h @@ -22,7 +22,7 @@ public: bool isReliable = false, bool isOrdered = false); PacketVersion getVersion() const { return _packetVersion; } - const QUuid& getSourceID() const { return _sourceID; } + NLPacket::LocalID getSourceID() const { return _sourceID; } qint64 getMaxSegmentSize() const override { return NLPacket::maxPayloadSize(_packetType, _isOrdered); } @@ -37,7 +37,7 @@ private: PacketVersion _packetVersion; - QUuid _sourceID; + NLPacket::LocalID _sourceID; }; Q_DECLARE_METATYPE(QSharedPointer) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 752d420a4b..4556b441f2 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -659,9 +659,8 @@ void NodeList::parseNodeFromPacketStream(QDataStream& packetStream) { packetStream >> connectionSecretUUID; - SharedNodePointer node = addOrUpdateNode(nodeUUID, nodeType, nodePublicSocket, - nodeLocalSocket, isReplicated, false, connectionSecretUUID, permissions); - node->setLocalID(sessionLocalID); + SharedNodePointer node = addOrUpdateNode(nodeUUID, nodeType, nodePublicSocket, nodeLocalSocket, + sessionLocalID, isReplicated, false, connectionSecretUUID, permissions); // nodes that are downstream or upstream of our own type are kept alive when we hear about them from the domain server // and always have their public socket as their active socket diff --git a/libraries/networking/src/PacketReceiver.cpp b/libraries/networking/src/PacketReceiver.cpp index 27b57ef26c..3d35424316 100644 --- a/libraries/networking/src/PacketReceiver.cpp +++ b/libraries/networking/src/PacketReceiver.cpp @@ -261,10 +261,7 @@ void PacketReceiver::handleVerifiedMessage(QSharedPointer recei SharedNodePointer matchingNode; - if (!receivedMessage->getSourceID().isNull()) { - matchingNode = nodeList->nodeWithUUID(receivedMessage->getSourceID()); - } - + matchingNode = nodeList->nodeWithLocalID(receivedMessage->getSourceID()); QMutexLocker packetListenerLocker(&_packetListenerLock); auto it = _messageListenerMap.find(receivedMessage->getType()); diff --git a/libraries/networking/src/ReceivedMessage.cpp b/libraries/networking/src/ReceivedMessage.cpp index 00b16908ce..e1a036b041 100644 --- a/libraries/networking/src/ReceivedMessage.cpp +++ b/libraries/networking/src/ReceivedMessage.cpp @@ -43,7 +43,7 @@ ReceivedMessage::ReceivedMessage(NLPacket& packet) } ReceivedMessage::ReceivedMessage(QByteArray byteArray, PacketType packetType, PacketVersion packetVersion, - const HifiSockAddr& senderSockAddr, QUuid sourceID) : + const HifiSockAddr& senderSockAddr, NLPacket::LocalID sourceID) : _data(byteArray), _headData(_data.mid(0, HEAD_DATA_SIZE)), _numPackets(1), diff --git a/libraries/networking/src/ReceivedMessage.h b/libraries/networking/src/ReceivedMessage.h index ae51e7592a..f94985b7d3 100644 --- a/libraries/networking/src/ReceivedMessage.h +++ b/libraries/networking/src/ReceivedMessage.h @@ -25,7 +25,7 @@ public: ReceivedMessage(const NLPacketList& packetList); ReceivedMessage(NLPacket& packet); ReceivedMessage(QByteArray byteArray, PacketType packetType, PacketVersion packetVersion, - const HifiSockAddr& senderSockAddr, QUuid sourceID = QUuid()); + const HifiSockAddr& senderSockAddr, NLPacket::LocalID sourceID = 0); QByteArray getMessage() const { return _data; } const char* getRawMessage() const { return _data.constData(); } @@ -40,7 +40,7 @@ public: bool failed() const { return _failed; } bool isComplete() const { return _isComplete; } - const QUuid& getSourceID() const { return _sourceID; } + NLPacket::LocalID getSourceID() const { return _sourceID; } const HifiSockAddr& getSenderSockAddr() { return _senderSockAddr; } qint64 getPosition() const { return _position; } @@ -93,7 +93,7 @@ private: std::atomic _position { 0 }; std::atomic _numPackets { 0 }; - QUuid _sourceID; + NLPacket::LocalID _sourceID; PacketType _packetType; PacketVersion _packetVersion; HifiSockAddr _senderSockAddr; diff --git a/libraries/octree/src/OctreeProcessor.cpp b/libraries/octree/src/OctreeProcessor.cpp index 65b30dd197..14c37d5116 100644 --- a/libraries/octree/src/OctreeProcessor.cpp +++ b/libraries/octree/src/OctreeProcessor.cpp @@ -96,7 +96,7 @@ void OctreeProcessor::processDatagram(ReceivedMessage& message, SharedNodePointe quint64 totalUncompress = 0; quint64 totalReadBitsteam = 0; - const QUuid& sourceUUID = message.getSourceID(); + const QUuid& sourceUUID = sourceNode->getUUID(); int subsection = 1; From bed403355461834e4768b3dfe459163df7826e6d Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 27 Mar 2018 18:18:14 -0700 Subject: [PATCH 04/15] Local node IDs now working correctly Move typedef to single location; fixes for replicated packets (probably still not correct); reserve zero as local ID; pass domain server's local ID in domain server list; other tweaks. --- assignment-client/src/audio/AudioMixer.cpp | 13 +++++-------- assignment-client/src/avatars/AvatarMixer.cpp | 10 +++------- domain-server/src/DomainGatekeeper.cpp | 2 +- domain-server/src/DomainServer.cpp | 4 +++- interface/src/octree/OctreePacketProcessor.cpp | 2 +- libraries/networking/src/LimitedNodeList.cpp | 4 ++-- libraries/networking/src/NLPacket.h | 4 ++-- libraries/networking/src/NetworkPeer.h | 3 ++- libraries/networking/src/NodeList.cpp | 3 +++ libraries/shared/src/UUID.h | 1 + 10 files changed, 23 insertions(+), 23 deletions(-) diff --git a/assignment-client/src/audio/AudioMixer.cpp b/assignment-client/src/audio/AudioMixer.cpp index 3b79eab06e..a50304e1d5 100644 --- a/assignment-client/src/audio/AudioMixer.cpp +++ b/assignment-client/src/audio/AudioMixer.cpp @@ -117,16 +117,13 @@ void AudioMixer::queueAudioPacket(QSharedPointer message, Share void AudioMixer::queueReplicatedAudioPacket(QSharedPointer message) { // make sure we have a replicated node for the original sender of the packet auto nodeList = DependencyManager::get(); - - QUuid nodeID; - SharedNodePointer sourceNode = nodeList->nodeWithLocalID(message->getSourceID()); - if (sourceNode) { - nodeID = sourceNode->getUUID(); - } + + // Node ID is now part of user data, since replicated audio packets are non-sourced. + QUuid nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); auto replicatedNode = nodeList->addOrUpdateNode(nodeID, NodeType::Agent, message->getSenderSockAddr(), message->getSenderSockAddr(), - true, true); + 0, true, true); replicatedNode->setLastHeardMicrostamp(usecTimestampNow()); // construct a "fake" audio received message from the byte array and packet list information @@ -140,7 +137,7 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess auto replicatedMessage = QSharedPointer::create(audioData, rewrittenType, versionForPacketType(rewrittenType), - message->getSenderSockAddr(), message->getSourceID()); + message->getSenderSockAddr(), nodeList->getSessionLocalID()); getOrCreateClientData(replicatedNode.data())->queuePacket(replicatedMessage, replicatedNode); } diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index b0f1420472..f9f41822a5 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -74,7 +74,7 @@ SharedNodePointer addOrUpdateReplicatedNode(const QUuid& nodeID, const HifiSockA auto replicatedNode = DependencyManager::get()->addOrUpdateNode(nodeID, NodeType::Agent, senderSockAddr, senderSockAddr, - true, true); + 0, true, true); replicatedNode->setLastHeardMicrostamp(usecTimestampNow()); @@ -112,12 +112,8 @@ void AvatarMixer::handleReplicatedPacket(QSharedPointer message void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer message) { while (message->getBytesLeftToRead()) { // first, grab the node ID for this replicated avatar - QUuid nodeID; - auto nodeList = DependencyManager::get(); - SharedNodePointer sourceNode = nodeList->nodeWithLocalID(message->getSourceID()); - if (sourceNode) { - nodeID = sourceNode->getUUID(); - } + // Node ID is now part of user data, since ReplicatedBulkAvatarPacket is non-sourced. + auto nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); // make sure we have an upstream replicated node that matches auto replicatedNode = addOrUpdateReplicatedNode(nodeID, message->getSenderSockAddr()); diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 917d8a01c0..40130688fb 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -1037,7 +1037,7 @@ Node::LocalID DomainGatekeeper::findOrCreateLocalID(const QUuid& uuid) { do { newLocalID = _currentLocalID; _currentLocalID += _idIncrement; - } while (_localIDToUUID.find(newLocalID) != _localIDToUUID.end()); + } while (newLocalID == 0 || _localIDToUUID.find(newLocalID) != _localIDToUUID.end()); _uuidToLocalID.emplace(uuid, newLocalID); _localIDToUUID.emplace(newLocalID, uuid); diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index fee102d66a..4514ed6fd6 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1159,7 +1159,8 @@ void DomainServer::handleConnectedNode(SharedNodePointer newNode) { } void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const HifiSockAddr &senderSockAddr) { - const int NUM_DOMAIN_LIST_EXTENDED_HEADER_BYTES = NUM_BYTES_RFC4122_UUID + NUM_BYTES_RFC4122_UUID + 2; + const int NUM_DOMAIN_LIST_EXTENDED_HEADER_BYTES = NUM_BYTES_RFC4122_UUID + NLPacket::NUM_BYTES_LOCALID + + NUM_BYTES_RFC4122_UUID + NLPacket::NUM_BYTES_LOCALID + 2; // setup the extended header for the domain list packets // this data is at the beginning of each of the domain list packets @@ -1169,6 +1170,7 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif auto limitedNodeList = DependencyManager::get(); extendedHeaderStream << limitedNodeList->getSessionUUID(); + extendedHeaderStream << limitedNodeList->getSessionLocalID(); extendedHeaderStream << node->getUUID(); extendedHeaderStream << node->getLocalID(); extendedHeaderStream << node->getPermissions(); diff --git a/interface/src/octree/OctreePacketProcessor.cpp b/interface/src/octree/OctreePacketProcessor.cpp index 122b58c057..0c2883a9a4 100644 --- a/interface/src/octree/OctreePacketProcessor.cpp +++ b/interface/src/octree/OctreePacketProcessor.cpp @@ -71,7 +71,7 @@ void OctreePacketProcessor::processPacket(QSharedPointer messag if (message->getVersion() != versionForPacketType(message->getType())) { static QMultiMap versionDebugSuppressMap; - const QUuid& senderUUID = message->getSourceID(); + const QUuid& senderUUID = sendingNode->getUUID(); if (!versionDebugSuppressMap.contains(senderUUID, packetType)) { qDebug() << "Was stats packet? " << wasStatsPacket; diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 77daaa84ea..e1f472e59c 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -315,10 +315,10 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe sourceNode = matchingNode.data(); } - QUuid sourceID = sourceNode->getUUID(); + QUuid sourceID = sourceNode ? sourceNode->getUUID() : QUuid(); if (!sourceNode && - sourceID == getDomainUUID() && + /*sourceID == getDomainUUID() &&*/ packet.getSenderSockAddr() == getDomainSockAddr() && PacketTypeEnum::getDomainSourcedPackets().contains(headerType)) { // This is a packet sourced by the domain server diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index a4690a376c..e3107fae3f 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -43,7 +43,7 @@ public: // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | // NLPacket Header Format - using LocalID = qint16; + using LocalID = NetworkLocalID; static const int NUM_BYTES_LOCALID = sizeof(LocalID); // this is used by the Octree classes - must be known at compile time static const int MAX_PACKET_HEADER_SIZE = @@ -83,7 +83,7 @@ public: LocalID getSourceID() const { return _sourceID; } - void writeSourceID(qint16 sourceID) const; + void writeSourceID(LocalID sourceID) const; void writeVerificationHashGivenSecret(const QUuid& connectionSecret) const; protected: diff --git a/libraries/networking/src/NetworkPeer.h b/libraries/networking/src/NetworkPeer.h index f36db402ce..972d2cbdd7 100644 --- a/libraries/networking/src/NetworkPeer.h +++ b/libraries/networking/src/NetworkPeer.h @@ -18,6 +18,7 @@ #include #include +#include "UUID.h" #include "HifiSockAddr.h" const QString ICE_SERVER_HOSTNAME = "localhost"; @@ -39,7 +40,7 @@ public: const QUuid& getUUID() const { return _uuid; } void setUUID(const QUuid& uuid) { _uuid = uuid; } - using LocalID = quint16; + using LocalID = NetworkLocalID; LocalID getLocalID() const { return _localID; } void setLocalID(LocalID localID) { _localID = localID; } diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 4556b441f2..86e3694669 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -594,6 +594,9 @@ void NodeList::processDomainServerList(QSharedPointer message) return; } + Node::LocalID domainLocalID; + packetStream >> domainLocalID; + // pull our owner (ie. session) UUID from the packet, it's always the first thing // The short (16 bit) ID comes next. QUuid newUUID; diff --git a/libraries/shared/src/UUID.h b/libraries/shared/src/UUID.h index 7e7048486f..55ac0866ee 100644 --- a/libraries/shared/src/UUID.h +++ b/libraries/shared/src/UUID.h @@ -15,6 +15,7 @@ #include const int NUM_BYTES_RFC4122_UUID = 16; +using NetworkLocalID = quint16; QString uuidStringWithoutCurlyBraces(const QUuid& uuid); From f823f632115c0d9db4a5a89ac5f82883342b7c91 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 28 Mar 2018 13:27:43 -0700 Subject: [PATCH 05/15] Add domain local ID to DomainServerList Also handle domain local ID for 'domain sourced' packets; bump packet version. --- domain-server/src/DomainServer.cpp | 1 - libraries/networking/src/DomainHandler.h | 4 ++++ libraries/networking/src/LimitedNodeList.cpp | 9 +++------ libraries/networking/src/LimitedNodeList.h | 1 + libraries/networking/src/NodeList.cpp | 1 + libraries/networking/src/NodeList.h | 1 + libraries/networking/src/udt/PacketHeaders.cpp | 2 +- 7 files changed, 11 insertions(+), 8 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 4514ed6fd6..4077becc57 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -593,7 +593,6 @@ bool DomainServer::isPacketVerified(const udt::Packet& packet) { if (!PacketTypeEnum::getNonSourcedPackets().contains(headerType)) { // this is a sourced packet - first check if we have a node that matches - //QUuid sourceID = NLPacket::sourceIDInHeader(packet); Node::LocalID localSourceID = NLPacket::sourceIDInHeader(packet); SharedNodePointer sourceNode = nodeList->nodeWithLocalID(localSourceID); diff --git a/libraries/networking/src/DomainHandler.h b/libraries/networking/src/DomainHandler.h index fbc60e2492..ef5b3116da 100644 --- a/libraries/networking/src/DomainHandler.h +++ b/libraries/networking/src/DomainHandler.h @@ -45,6 +45,9 @@ public: const QUuid& getUUID() const { return _uuid; } void setUUID(const QUuid& uuid); + const Node::LocalID getLocalID() const { return _localID; } + void setLocalID(Node::LocalID localID) { _localID = localID; } + QString getHostname() const { return _domainURL.host(); } const QHostAddress& getIP() const { return _sockAddr.getAddress(); } @@ -181,6 +184,7 @@ private: void hardReset(); QUuid _uuid; + Node::LocalID _localID; QUrl _domainURL; HifiSockAddr _sockAddr; QUuid _assignmentUUID; diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index e1f472e59c..31345c3c14 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -305,11 +305,11 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe return true; } } else { - + NLPacket::LocalID sourceLocalID = 0; // check if we were passed a sourceNode hint or if we need to look it up if (!sourceNode) { // figure out which node this is from - NLPacket::LocalID sourceLocalID = NLPacket::sourceIDInHeader(packet); + sourceLocalID = NLPacket::sourceIDInHeader(packet); SharedNodePointer matchingNode = nodeWithLocalID(sourceLocalID); sourceNode = matchingNode.data(); @@ -318,7 +318,7 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe QUuid sourceID = sourceNode ? sourceNode->getUUID() : QUuid(); if (!sourceNode && - /*sourceID == getDomainUUID() &&*/ + sourceLocalID == getDomainLocalID() && packet.getSenderSockAddr() == getDomainSockAddr() && PacketTypeEnum::getDomainSourcedPackets().contains(headerType)) { // This is a packet sourced by the domain server @@ -567,9 +567,6 @@ SharedNodePointer LimitedNodeList::nodeWithLocalID(Node::LocalID localID) const QReadLocker readLocker(&_nodeMutex); LocalIDMapping::const_iterator idIter = _localIDMap.find(localID); - if (idIter == _localIDMap.cend()) { - qCDebug(networking) << "No such Node with local ID " << localID; - } return idIter == _localIDMap.cend() ? nullptr : idIter->second; } diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 51c8831e2b..95d0e8b559 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -128,6 +128,7 @@ public: virtual bool isDomainServer() const { return true; } virtual QUuid getDomainUUID() const { assert(false); return QUuid(); } + virtual Node::LocalID getDomainLocalID() const { assert(false); return 0; } virtual HifiSockAddr getDomainSockAddr() const { assert(false); return HifiSockAddr(); } // use sendUnreliablePacket to send an unrelaible packet (that you do not need to move) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 86e3694669..4b3595b279 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -608,6 +608,7 @@ void NodeList::processDomainServerList(QSharedPointer message) // if this was the first domain-server list from this domain, we've now connected if (!_domainHandler.isConnected()) { + _domainHandler.setLocalID(newLocalID); _domainHandler.setUUID(domainUUID); _domainHandler.setIsConnected(true); diff --git a/libraries/networking/src/NodeList.h b/libraries/networking/src/NodeList.h index 7e3a7c2bd7..9595c5da84 100644 --- a/libraries/networking/src/NodeList.h +++ b/libraries/networking/src/NodeList.h @@ -94,6 +94,7 @@ public: virtual bool isDomainServer() const override { return false; } virtual QUuid getDomainUUID() const override { return _domainHandler.getUUID(); } + virtual Node::LocalID getDomainLocalID() const override { return _domainHandler.getLocalID(); } virtual HifiSockAddr getDomainSockAddr() const override { return _domainHandler.getSockAddr(); } public slots: diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index a83924ee58..8a3592de45 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -76,7 +76,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::DomainSettings: return 18; // replace min_avatar_scale and max_avatar_scale with min_avatar_height and max_avatar_height default: - return 17; + return 19; } } From b409e04734d4240968fe221981ceac70fd659630 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 28 Mar 2018 17:42:23 -0700 Subject: [PATCH 06/15] Handle all case of nodes being deleted from nodelist Delete erased nodes from local ID mapping, as this uses SharedPointer. Don't keep the mapping from local IDs to UUIDs in GateKeeper as it isn't used. --- domain-server/src/DomainGatekeeper.cpp | 6 ++++-- domain-server/src/DomainGatekeeper.h | 5 +++-- domain-server/src/DomainServer.cpp | 2 +- libraries/networking/src/LimitedNodeList.cpp | 4 ++++ libraries/networking/src/NLPacket.h | 18 +++++++++++++++++- 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 40130688fb..fb3c305b84 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -1033,13 +1033,15 @@ Node::LocalID DomainGatekeeper::findOrCreateLocalID(const QUuid& uuid) { return existingLocalIDIt->second; } + assert(_localIDs.size() < std::numeric_limits::max() - 2); + Node::LocalID newLocalID; do { newLocalID = _currentLocalID; _currentLocalID += _idIncrement; - } while (newLocalID == 0 || _localIDToUUID.find(newLocalID) != _localIDToUUID.end()); + } while (newLocalID == 0 || _localIDs.find(newLocalID) != _localIDs.end()); _uuidToLocalID.emplace(uuid, newLocalID); - _localIDToUUID.emplace(newLocalID, uuid); + _localIDs.insert(newLocalID); return newLocalID; } diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index afb848f271..edd976a77b 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -15,6 +15,7 @@ #define hifi_DomainGatekeeper_h #include +#include #include #include @@ -129,9 +130,9 @@ private: size_t operator()(const QUuid& uuid) const { return qHash(uuid); } }; using UUIDToLocalID = std::unordered_map ; - using LocalIDToUUID = std::unordered_map; + using LocalIDs = std::unordered_set; + LocalIDs _localIDs; UUIDToLocalID _uuidToLocalID; - LocalIDToUUID _localIDToUUID; Node::LocalID _currentLocalID; quint16 _idIncrement; diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 4077becc57..77f25d4c7e 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -2905,7 +2905,7 @@ void DomainServer::updateReplicationNodes(ReplicationServerDirection direction) // manually add the replication node to our node list auto node = nodeList->addOrUpdateNode(QUuid::createUuid(), replicationServer.nodeType, replicationServer.sockAddr, replicationServer.sockAddr, - false, direction == Upstream); + 0, false, direction == Upstream); node->setIsForcedNeverSilent(true); qDebug() << "Adding" << (direction == Upstream ? "upstream" : "downstream") diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 31345c3c14..6ef119bf3b 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -695,6 +695,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t auto oldSoloNode = previousSoloIt->second; + _localIDMap.erase(oldSoloNode->getLocalID()); _nodeHash.unsafe_erase(previousSoloIt); handleNodeKill(oldSoloNode); @@ -850,6 +851,9 @@ void LimitedNodeList::removeSilentNodes() { }); foreach(const SharedNodePointer& killedNode, killedNodes) { + _nodeMutex.lockForWrite(); + _localIDMap.erase(killedNode->getLocalID()); + _nodeMutex.unlock(); handleNodeKill(killedNode); } } diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index e3107fae3f..a994b7bde0 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -21,6 +21,22 @@ class NLPacket : public udt::Packet { Q_OBJECT public: + // + // Current NLPacket format: + // + // | BYTE | BYTE | BYTE | BYTE | + // 0 1 2 3 + // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + // | Packet Type | Version | Local Node ID - sourced only | + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + // | | + // | MD5 Verification - 16 bytes | + // | (ONLY FOR VERIFIED PACKETS) | + // | | + // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + + // Old NLPacket format: // // | BYTE | BYTE | BYTE | BYTE | // @@ -41,7 +57,7 @@ public: // | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ // | | | // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | - // NLPacket Header Format + // using LocalID = NetworkLocalID; static const int NUM_BYTES_LOCALID = sizeof(LocalID); From 1b0b280f3a17d55f5a55e043066c768dcd56d9eb Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 29 Mar 2018 10:47:11 -0700 Subject: [PATCH 07/15] Fix SequenceNumberStats, misc. clean-up --- domain-server/src/DomainGatekeeper.cpp | 2 +- domain-server/src/DomainGatekeeper.h | 2 +- domain-server/src/DomainServer.cpp | 2 +- libraries/audio/src/InboundAudioStream.cpp | 4 ++-- libraries/networking/src/LimitedNodeList.cpp | 2 +- libraries/networking/src/SequenceNumberStats.cpp | 12 ++++++------ libraries/networking/src/SequenceNumberStats.h | 6 +++--- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index fb3c305b84..b4ce467e0a 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -1021,7 +1021,7 @@ void DomainGatekeeper::refreshGroupsCache() { void DomainGatekeeper::initLocalIDManagement() { std::uniform_int_distribution sixteenBitRand; std::random_device randomDevice; - std::default_random_engine engine {randomDevice()}; + std::default_random_engine engine { randomDevice() }; _currentLocalID = sixteenBitRand(engine); // Ensure increment is odd. _idIncrement = sixteenBitRand(engine) | 1; diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index edd976a77b..ec7ec5701f 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -135,7 +135,7 @@ private: UUIDToLocalID _uuidToLocalID; Node::LocalID _currentLocalID; - quint16 _idIncrement; + Node::LocalID _idIncrement; }; diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 77f25d4c7e..7265dff15c 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -1159,7 +1159,7 @@ void DomainServer::handleConnectedNode(SharedNodePointer newNode) { void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const HifiSockAddr &senderSockAddr) { const int NUM_DOMAIN_LIST_EXTENDED_HEADER_BYTES = NUM_BYTES_RFC4122_UUID + NLPacket::NUM_BYTES_LOCALID + - NUM_BYTES_RFC4122_UUID + NLPacket::NUM_BYTES_LOCALID + 2; + NUM_BYTES_RFC4122_UUID + NLPacket::NUM_BYTES_LOCALID + 4; // setup the extended header for the domain list packets // this data is at the beginning of each of the domain list packets diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 983b5e1cb8..172ec9411a 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -120,8 +120,8 @@ int InboundAudioStream::parseData(ReceivedMessage& message) { // parse sequence number and track it quint16 sequence; message.readPrimitive(&sequence); - SequenceNumberStats::ArrivalInfo arrivalInfo = _incomingSequenceNumberStats.sequenceNumberReceived(sequence, QUuid() // TBD - /*message.getSourceID()*/); + SequenceNumberStats::ArrivalInfo arrivalInfo = + _incomingSequenceNumberStats.sequenceNumberReceived(sequence, message.getSourceID()); QString codecInPacket = message.readString(); packetReceivedUpdateTimingStats(); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 6ef119bf3b..177bc729cd 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -136,7 +136,7 @@ Node::LocalID LimitedNodeList::getSessionLocalID() const { } void LimitedNodeList::setSessionLocalID(Node::LocalID sessionLocalID) { - QWriteLocker lock { &_sessionUUIDLock }; // Necessary? + QWriteLocker lock { &_sessionUUIDLock }; _sessionLocalID = sessionLocalID; } diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 6d7b271606..052cdf1a4e 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -25,7 +25,7 @@ SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, bool canDetectO : _lastReceivedSequence(0), _missingSet(), _stats(), - _lastSenderUUID(), + _lastSenderID(0), _statsHistory(statsHistoryLength), _lastUnreasonableSequence(0), _consecutiveUnreasonableOnTime(0) @@ -35,7 +35,7 @@ SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, bool canDetectO void SequenceNumberStats::reset() { _missingSet.clear(); _stats = PacketStreamStats(); - _lastSenderUUID = QUuid(); + _lastSenderID = 0; _statsHistory.clear(); _lastUnreasonableSequence = 0; _consecutiveUnreasonableOnTime = 0; @@ -43,18 +43,18 @@ void SequenceNumberStats::reset() { static const int UINT16_RANGE = std::numeric_limits::max() + 1; -SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(quint16 incoming, QUuid senderUUID, const bool wantExtraDebugging) { +SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(quint16 incoming, NetworkLocalID senderID, const bool wantExtraDebugging) { SequenceNumberStats::ArrivalInfo arrivalInfo; // if the sender node has changed, reset all stats - if (senderUUID != _lastSenderUUID) { + if (senderID != _lastSenderID) { if (_stats._received > 0) { qCDebug(networking) << "sequence number stats was reset due to new sender node"; - qCDebug(networking) << "previous:" << _lastSenderUUID << "current:" << senderUUID; + qCDebug(networking) << "previous:" << _lastSenderID << "current:" << senderID; reset(); } - _lastSenderUUID = senderUUID; + _lastSenderID = senderID; } // determine our expected sequence number... handle rollover appropriately diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index 46149d4307..ba9db2a225 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -14,7 +14,7 @@ #include "SharedUtil.h" #include "RingBufferHistory.h" -#include +#include "UUID.h" const int MAX_REASONABLE_SEQUENCE_GAP = 1000; @@ -73,7 +73,7 @@ public: SequenceNumberStats(int statsHistoryLength = 0, bool canDetectOutOfSync = true); void reset(); - ArrivalInfo sequenceNumberReceived(quint16 incoming, QUuid senderUUID = QUuid(), const bool wantExtraDebugging = false); + ArrivalInfo sequenceNumberReceived(quint16 incoming, NetworkLocalID senderID = 0, const bool wantExtraDebugging = false); void pruneMissingSet(const bool wantExtraDebugging = false); void pushStatsToHistory() { _statsHistory.insert(_stats); } @@ -100,7 +100,7 @@ private: PacketStreamStats _stats; - QUuid _lastSenderUUID; + NetworkLocalID _lastSenderID; RingBufferHistory _statsHistory; From e9ec4df36f75ff54577c9ddcb76a5032077a18fe Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 29 Mar 2018 14:10:31 -0700 Subject: [PATCH 08/15] Gcc complained about superfluous const qualifier --- libraries/networking/src/DomainHandler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/DomainHandler.h b/libraries/networking/src/DomainHandler.h index ef5b3116da..c447e9a79b 100644 --- a/libraries/networking/src/DomainHandler.h +++ b/libraries/networking/src/DomainHandler.h @@ -45,7 +45,7 @@ public: const QUuid& getUUID() const { return _uuid; } void setUUID(const QUuid& uuid); - const Node::LocalID getLocalID() const { return _localID; } + Node::LocalID getLocalID() const { return _localID; } void setLocalID(Node::LocalID localID) { _localID = localID; } QString getHostname() const { return _domainURL.host(); } From 734b48eee0f02ad8a0b30f4ad6fd5908d2feaa44 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 30 Mar 2018 17:27:52 -0700 Subject: [PATCH 09/15] Local IDs - Reviewer's suggested improvements --- assignment-client/src/audio/AudioMixer.cpp | 2 +- libraries/networking/src/LimitedNodeList.cpp | 10 ++++---- libraries/networking/src/NLPacket.h | 24 +------------------- libraries/networking/src/PacketReceiver.cpp | 4 +++- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/assignment-client/src/audio/AudioMixer.cpp b/assignment-client/src/audio/AudioMixer.cpp index a50304e1d5..a18df68f90 100644 --- a/assignment-client/src/audio/AudioMixer.cpp +++ b/assignment-client/src/audio/AudioMixer.cpp @@ -137,7 +137,7 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess auto replicatedMessage = QSharedPointer::create(audioData, rewrittenType, versionForPacketType(rewrittenType), - message->getSenderSockAddr(), nodeList->getSessionLocalID()); + message->getSenderSockAddr(), message->getSourceID()); getOrCreateClientData(replicatedNode.data())->queuePacket(replicatedMessage, replicatedNode); } diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 177bc729cd..c30952c344 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -132,6 +132,7 @@ void LimitedNodeList::setSessionUUID(const QUuid& sessionUUID) { } Node::LocalID LimitedNodeList::getSessionLocalID() const { + QReadLocker readLock { &_sessionUUIDLock }; return _sessionLocalID; } @@ -578,7 +579,7 @@ void LimitedNodeList::eraseAllNodes() { // and then remove them from the hash QWriteLocker writeLocker(&_nodeMutex); - _localIDMap.erase(_localIDMap.begin(), _localIDMap.end()); + _localIDMap.clear(); if (_nodeHash.size() > 0) { qCDebug(networking) << "LimitedNodeList::eraseAllNodes() removing all nodes from NodeList."; @@ -851,9 +852,10 @@ void LimitedNodeList::removeSilentNodes() { }); foreach(const SharedNodePointer& killedNode, killedNodes) { - _nodeMutex.lockForWrite(); - _localIDMap.erase(killedNode->getLocalID()); - _nodeMutex.unlock(); + { + QWriteLocker writeLock { &_nodeMutex }; + _localIDMap.erase(killedNode->getLocalID()); + } handleNodeKill(killedNode); } } diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index a994b7bde0..b9c598500b 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -22,7 +22,7 @@ class NLPacket : public udt::Packet { Q_OBJECT public: // - // Current NLPacket format: + // NLPacket format: // // | BYTE | BYTE | BYTE | BYTE | // 0 1 2 3 @@ -35,28 +35,6 @@ public: // | (ONLY FOR VERIFIED PACKETS) | // | | // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - - // Old NLPacket format: - // - // | BYTE | BYTE | BYTE | BYTE | - // - // 0 1 2 3 - // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 - // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - // | Packet Type | Version | | - // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | - // | | - // | Node UUID - 16 bytes | - // | (ONLY FOR SOURCED PACKETS) | - // | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - // | | | - // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | - // | | - // | MD5 Verification - 16 bytes | - // | (ONLY FOR VERIFIED PACKETS) | - // | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - // | | | - // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | // using LocalID = NetworkLocalID; diff --git a/libraries/networking/src/PacketReceiver.cpp b/libraries/networking/src/PacketReceiver.cpp index 3d35424316..39a3800c4b 100644 --- a/libraries/networking/src/PacketReceiver.cpp +++ b/libraries/networking/src/PacketReceiver.cpp @@ -261,7 +261,9 @@ void PacketReceiver::handleVerifiedMessage(QSharedPointer recei SharedNodePointer matchingNode; - matchingNode = nodeList->nodeWithLocalID(receivedMessage->getSourceID()); + if (receivedMessage->getSourceID() != 0) { + matchingNode = nodeList->nodeWithLocalID(receivedMessage->getSourceID()); + } QMutexLocker packetListenerLocker(&_packetListenerLock); auto it = _messageListenerMap.find(receivedMessage->getType()); From efb1fdbc0d2597ff96e60292eff93cac454d9bf6 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 30 Mar 2018 18:29:26 -0700 Subject: [PATCH 10/15] Local IDs - add an explicit null value, use it for replicated packets --- assignment-client/src/audio/AudioMixer.cpp | 2 +- assignment-client/src/avatars/AvatarMixer.cpp | 2 +- domain-server/src/DomainGatekeeper.cpp | 2 +- libraries/networking/src/NLPacket.h | 2 ++ libraries/networking/src/NetworkPeer.h | 2 ++ libraries/networking/src/ReceivedMessage.h | 4 ++-- 6 files changed, 9 insertions(+), 5 deletions(-) diff --git a/assignment-client/src/audio/AudioMixer.cpp b/assignment-client/src/audio/AudioMixer.cpp index a18df68f90..d47172f62e 100644 --- a/assignment-client/src/audio/AudioMixer.cpp +++ b/assignment-client/src/audio/AudioMixer.cpp @@ -137,7 +137,7 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess auto replicatedMessage = QSharedPointer::create(audioData, rewrittenType, versionForPacketType(rewrittenType), - message->getSenderSockAddr(), message->getSourceID()); + message->getSenderSockAddr(), Node::NULL_LOCAL_ID); getOrCreateClientData(replicatedNode.data())->queuePacket(replicatedMessage, replicatedNode); } diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index f9f41822a5..25bbc4cd0a 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -127,7 +127,7 @@ void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer::create(avatarByteArray, PacketType::AvatarData, versionForPacketType(PacketType::AvatarData), - message->getSenderSockAddr(), message->getSourceID()); + message->getSenderSockAddr(), Node::NULL_LOCAL_ID); // queue up the replicated avatar data with the client data for the replicated node auto start = usecTimestampNow(); diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index b4ce467e0a..25317bf44b 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -1039,7 +1039,7 @@ Node::LocalID DomainGatekeeper::findOrCreateLocalID(const QUuid& uuid) { do { newLocalID = _currentLocalID; _currentLocalID += _idIncrement; - } while (newLocalID == 0 || _localIDs.find(newLocalID) != _localIDs.end()); + } while (newLocalID == Node::NULL_LOCAL_ID || _localIDs.find(newLocalID) != _localIDs.end()); _uuidToLocalID.emplace(uuid, newLocalID); _localIDs.insert(newLocalID); diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index b9c598500b..edd03627c3 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -38,6 +38,8 @@ public: // using LocalID = NetworkLocalID; + static const LocalID NULL_LOCAL_ID = 0; + static const int NUM_BYTES_LOCALID = sizeof(LocalID); // this is used by the Octree classes - must be known at compile time static const int MAX_PACKET_HEADER_SIZE = diff --git a/libraries/networking/src/NetworkPeer.h b/libraries/networking/src/NetworkPeer.h index 972d2cbdd7..462daa1ed2 100644 --- a/libraries/networking/src/NetworkPeer.h +++ b/libraries/networking/src/NetworkPeer.h @@ -41,6 +41,8 @@ public: void setUUID(const QUuid& uuid) { _uuid = uuid; } using LocalID = NetworkLocalID; + static const LocalID NULL_LOCAL_ID = 0; + LocalID getLocalID() const { return _localID; } void setLocalID(LocalID localID) { _localID = localID; } diff --git a/libraries/networking/src/ReceivedMessage.h b/libraries/networking/src/ReceivedMessage.h index f94985b7d3..af87ef75af 100644 --- a/libraries/networking/src/ReceivedMessage.h +++ b/libraries/networking/src/ReceivedMessage.h @@ -25,7 +25,7 @@ public: ReceivedMessage(const NLPacketList& packetList); ReceivedMessage(NLPacket& packet); ReceivedMessage(QByteArray byteArray, PacketType packetType, PacketVersion packetVersion, - const HifiSockAddr& senderSockAddr, NLPacket::LocalID sourceID = 0); + const HifiSockAddr& senderSockAddr, NLPacket::LocalID sourceID = NLPacket::NULL_LOCAL_ID); QByteArray getMessage() const { return _data; } const char* getRawMessage() const { return _data.constData(); } @@ -93,7 +93,7 @@ private: std::atomic _position { 0 }; std::atomic _numPackets { 0 }; - NLPacket::LocalID _sourceID; + NLPacket::LocalID _sourceID { NLPacket::NULL_LOCAL_ID }; PacketType _packetType; PacketVersion _packetVersion; HifiSockAddr _senderSockAddr; From f4bd1f5ed7130d3272a45f5f6dfbc8253ae542be Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Mon, 2 Apr 2018 09:45:46 -0700 Subject: [PATCH 11/15] Add definition for NetworkPeer::NULL_LOCAL_ID to (hopefully) satisfy MacOS --- libraries/networking/src/NetworkPeer.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/networking/src/NetworkPeer.cpp b/libraries/networking/src/NetworkPeer.cpp index b52c54c468..6d384ba558 100644 --- a/libraries/networking/src/NetworkPeer.cpp +++ b/libraries/networking/src/NetworkPeer.cpp @@ -23,6 +23,7 @@ #include #include "NodeType.h" +const NetworkPeer::LocalID NetworkPeer::NULL_LOCAL_ID; NetworkPeer::NetworkPeer(QObject* parent) : QObject(parent), From 2ba64e111509e2d3c77c2b860120856806f7648e Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Mon, 2 Apr 2018 10:46:38 -0700 Subject: [PATCH 12/15] LocalID - Use tbb hash map instead of std one for better concurrency --- libraries/networking/src/LimitedNodeList.cpp | 7 ++----- libraries/networking/src/LimitedNodeList.h | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index c30952c344..ec2e107567 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -696,7 +696,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t auto oldSoloNode = previousSoloIt->second; - _localIDMap.erase(oldSoloNode->getLocalID()); + _localIDMap.unsafe_erase(oldSoloNode->getLocalID()); _nodeHash.unsafe_erase(previousSoloIt); handleNodeKill(oldSoloNode); @@ -840,6 +840,7 @@ void LimitedNodeList::removeSilentNodes() { if (!node->isForcedNeverSilent() && (usecTimestampNow() - node->getLastHeardMicrostamp()) > (NODE_SILENCE_THRESHOLD_MSECS * USECS_PER_MSEC)) { // call the NodeHash erase to get rid of this node + _localIDMap.unsafe_erase(node->getLocalID()); it = _nodeHash.unsafe_erase(it); killedNodes.insert(node); @@ -852,10 +853,6 @@ void LimitedNodeList::removeSilentNodes() { }); foreach(const SharedNodePointer& killedNode, killedNodes) { - { - QWriteLocker writeLock { &_nodeMutex }; - _localIDMap.erase(killedNode->getLocalID()); - } handleNodeKill(killedNode); } } diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 95d0e8b559..6c61fbc43c 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -431,7 +431,7 @@ private slots: private: mutable QReadWriteLock _sessionUUIDLock; QUuid _sessionUUID; - using LocalIDMapping = std::unordered_map; + using LocalIDMapping = tbb::concurrent_unordered_map; LocalIDMapping _localIDMap; Node::LocalID _sessionLocalID { 0 }; }; From 01c39d4310484d4b1d930de8af26240cf701d186 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 18 Apr 2018 11:53:47 -0700 Subject: [PATCH 13/15] Use NULL_LOCAL_ID in more places, as requested by review --- assignment-client/src/audio/AudioMixer.cpp | 2 +- domain-server/src/DomainGatekeeper.h | 5 +---- domain-server/src/DomainServer.cpp | 2 +- libraries/networking/src/LimitedNodeList.cpp | 2 +- libraries/networking/src/LimitedNodeList.h | 6 +++--- libraries/networking/src/PacketReceiver.cpp | 2 +- libraries/networking/src/SequenceNumberStats.cpp | 4 ++-- libraries/networking/src/SequenceNumberStats.h | 3 ++- 8 files changed, 12 insertions(+), 14 deletions(-) diff --git a/assignment-client/src/audio/AudioMixer.cpp b/assignment-client/src/audio/AudioMixer.cpp index d47172f62e..34eb138697 100644 --- a/assignment-client/src/audio/AudioMixer.cpp +++ b/assignment-client/src/audio/AudioMixer.cpp @@ -123,7 +123,7 @@ void AudioMixer::queueReplicatedAudioPacket(QSharedPointer mess auto replicatedNode = nodeList->addOrUpdateNode(nodeID, NodeType::Agent, message->getSenderSockAddr(), message->getSenderSockAddr(), - 0, true, true); + Node::NULL_LOCAL_ID, true, true); replicatedNode->setLastHeardMicrostamp(usecTimestampNow()); // construct a "fake" audio received message from the byte array and packet list information diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index ec7ec5701f..8402e58559 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -126,10 +126,7 @@ private: // Local ID management. void initLocalIDManagement(); - struct UuidHash { - size_t operator()(const QUuid& uuid) const { return qHash(uuid); } - }; - using UUIDToLocalID = std::unordered_map ; + using UUIDToLocalID = std::unordered_map ; using LocalIDs = std::unordered_set; LocalIDs _localIDs; UUIDToLocalID _uuidToLocalID; diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 7265dff15c..2fa86cc860 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -2905,7 +2905,7 @@ void DomainServer::updateReplicationNodes(ReplicationServerDirection direction) // manually add the replication node to our node list auto node = nodeList->addOrUpdateNode(QUuid::createUuid(), replicationServer.nodeType, replicationServer.sockAddr, replicationServer.sockAddr, - 0, false, direction == Upstream); + Node::NULL_LOCAL_ID, false, direction == Upstream); node->setIsForcedNeverSilent(true); qDebug() << "Adding" << (direction == Upstream ? "upstream" : "downstream") diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index ec2e107567..88bf45a419 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -306,7 +306,7 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe return true; } } else { - NLPacket::LocalID sourceLocalID = 0; + NLPacket::LocalID sourceLocalID = Node::NULL_LOCAL_ID; // check if we were passed a sourceNode hint or if we need to look it up if (!sourceNode) { // figure out which node this is from diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 6c61fbc43c..c91c012f27 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -128,7 +128,7 @@ public: virtual bool isDomainServer() const { return true; } virtual QUuid getDomainUUID() const { assert(false); return QUuid(); } - virtual Node::LocalID getDomainLocalID() const { assert(false); return 0; } + virtual Node::LocalID getDomainLocalID() const { assert(false); return Node::NULL_LOCAL_ID; } virtual HifiSockAddr getDomainSockAddr() const { assert(false); return HifiSockAddr(); } // use sendUnreliablePacket to send an unrelaible packet (that you do not need to move) @@ -162,8 +162,8 @@ public: SharedNodePointer addOrUpdateNode(const QUuid& uuid, NodeType_t nodeType, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, - Node::LocalID localID = 0, bool isReplicated = false, bool isUpstream = false, - const QUuid& connectionSecret = QUuid(), + Node::LocalID localID = Node::NULL_LOCAL_ID, bool isReplicated = false, + bool isUpstream = false, const QUuid& connectionSecret = QUuid(), const NodePermissions& permissions = DEFAULT_AGENT_PERMISSIONS); static bool parseSTUNResponse(udt::BasePacket* packet, QHostAddress& newPublicAddress, uint16_t& newPublicPort); diff --git a/libraries/networking/src/PacketReceiver.cpp b/libraries/networking/src/PacketReceiver.cpp index 39a3800c4b..fe2a273d61 100644 --- a/libraries/networking/src/PacketReceiver.cpp +++ b/libraries/networking/src/PacketReceiver.cpp @@ -261,7 +261,7 @@ void PacketReceiver::handleVerifiedMessage(QSharedPointer recei SharedNodePointer matchingNode; - if (receivedMessage->getSourceID() != 0) { + if (receivedMessage->getSourceID() != Node::NULL_LOCAL_ID) { matchingNode = nodeList->nodeWithLocalID(receivedMessage->getSourceID()); } QMutexLocker packetListenerLocker(&_packetListenerLock); diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 052cdf1a4e..03e149eb0e 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -25,7 +25,7 @@ SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, bool canDetectO : _lastReceivedSequence(0), _missingSet(), _stats(), - _lastSenderID(0), + _lastSenderID(NULL_LOCAL_ID), _statsHistory(statsHistoryLength), _lastUnreasonableSequence(0), _consecutiveUnreasonableOnTime(0) @@ -35,7 +35,7 @@ SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, bool canDetectO void SequenceNumberStats::reset() { _missingSet.clear(); _stats = PacketStreamStats(); - _lastSenderID = 0; + _lastSenderID = NULL_LOCAL_ID; _statsHistory.clear(); _lastUnreasonableSequence = 0; _consecutiveUnreasonableOnTime = 0; diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index ba9db2a225..8705f960b7 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -73,7 +73,7 @@ public: SequenceNumberStats(int statsHistoryLength = 0, bool canDetectOutOfSync = true); void reset(); - ArrivalInfo sequenceNumberReceived(quint16 incoming, NetworkLocalID senderID = 0, const bool wantExtraDebugging = false); + ArrivalInfo sequenceNumberReceived(quint16 incoming, NetworkLocalID senderID = NULL_LOCAL_ID, const bool wantExtraDebugging = false); void pruneMissingSet(const bool wantExtraDebugging = false); void pushStatsToHistory() { _statsHistory.insert(_stats); } @@ -101,6 +101,7 @@ private: PacketStreamStats _stats; NetworkLocalID _lastSenderID; + static const NetworkLocalID NULL_LOCAL_ID = (NetworkLocalID) 0; RingBufferHistory _statsHistory; From c964852e45436643e4ba43fa13bf93114b6eefde Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 18 Apr 2018 15:09:00 -0700 Subject: [PATCH 14/15] Another use of NULL_LOCAL_ID --- assignment-client/src/avatars/AvatarMixer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 25bbc4cd0a..29340f6474 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -74,7 +74,7 @@ SharedNodePointer addOrUpdateReplicatedNode(const QUuid& nodeID, const HifiSockA auto replicatedNode = DependencyManager::get()->addOrUpdateNode(nodeID, NodeType::Agent, senderSockAddr, senderSockAddr, - 0, true, true); + Node::NULL_LOCAL_ID, true, true); replicatedNode->setLastHeardMicrostamp(usecTimestampNow()); From ed0fd8c4d21f626dd650190e523dedecfaff6eda Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 20 Apr 2018 11:13:15 -0700 Subject: [PATCH 15/15] Merge fix --- domain-server/src/DomainServer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index d5f3c99b38..96de8f7f12 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -612,13 +612,13 @@ bool DomainServer::isPacketVerified(const udt::Packet& packet) { return nodeList->isPacketVerifiedWithSource(packet, sourceNode.data()); } else { HIFI_FDEBUG("Packet of type" << headerType - << "received from unmatched IP for UUID" << uuidStringWithoutCurlyBraces(sourceID)); + << "received from unmatched IP for UUID" << uuidStringWithoutCurlyBraces(sourceNode->getUUID())); return false; } } else { HIFI_FDEBUG("Packet of type" << headerType - << "received from unknown node with UUID" << uuidStringWithoutCurlyBraces(sourceID)); + << "received from unknown node with UUID" << uuidStringWithoutCurlyBraces(sourceNode->getUUID())); return false; }