From 61eb93af357e500123ea6feb4377931f2da252d3 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 6 Jun 2017 19:04:12 -0700 Subject: [PATCH 1/4] Fix SharedNodePointer leak --- libraries/networking/src/LimitedNodeList.cpp | 18 +++++++++++------- libraries/networking/src/Node.h | 2 +- .../networking/src/ReceivedPacketProcessor.h | 4 +++- libraries/shared/src/GenericThread.cpp | 2 ++ 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 93ae941f1e..f9baff0daf 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -584,7 +584,7 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t return matchingNode; } else { // we didn't have this node, so add them - Node* newNode = new Node(uuid, nodeType, publicSocket, localSocket, permissions, connectionSecret, this); + Node* newNode = new Node(uuid, nodeType, publicSocket, localSocket, permissions, connectionSecret); if (nodeType == NodeType::AudioMixer) { LimitedNodeList::flagTimeForConnectionStep(LimitedNodeList::AddedAudioMixer); @@ -617,24 +617,28 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t } // insert the new node and release our read lock - _nodeHash.insert(UUIDNodePair(newNode->getUUID(), newNodePointer)); + _nodeHash.emplace(newNode->getUUID(), newNodePointer); readLocker.unlock(); qCDebug(networking) << "Added" << *newNode; + auto weakPtr = newNodePointer.toWeakRef(); // We don't want the lambdas to hold a strong ref + emit nodeAdded(newNodePointer); if (newNodePointer->getActiveSocket()) { emit nodeActivated(newNodePointer); } else { - connect(newNodePointer.data(), &NetworkPeer::socketActivated, this, [=] { - emit nodeActivated(newNodePointer); - disconnect(newNodePointer.data(), &NetworkPeer::socketActivated, this, 0); + connect(newNodePointer.data(), &NetworkPeer::socketActivated, this, [this, weakPtr] { + auto sharedPtr = weakPtr.lock(); + if (sharedPtr) { + emit nodeActivated(sharedPtr); + disconnect(sharedPtr.data(), &NetworkPeer::socketActivated, this, 0); + } }); } // Signal when a socket changes, so we can start the hole punch over. - auto weakPtr = newNodePointer.toWeakRef(); // We don't want the lambda to hold a strong ref - connect(newNodePointer.data(), &NetworkPeer::socketUpdated, this, [=] { + connect(newNodePointer.data(), &NetworkPeer::socketUpdated, this, [this, weakPtr] { emit nodeSocketUpdated(weakPtr); }); diff --git a/libraries/networking/src/Node.h b/libraries/networking/src/Node.h index 1092fcc7fa..1bb3a0cdc8 100644 --- a/libraries/networking/src/Node.h +++ b/libraries/networking/src/Node.h @@ -40,7 +40,7 @@ public: Node(const QUuid& uuid, NodeType_t type, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, const NodePermissions& permissions, const QUuid& connectionSecret = QUuid(), - QObject* parent = 0); + QObject* parent = nullptr); bool operator==(const Node& otherNode) const { return _uuid == otherNode._uuid; } bool operator!=(const Node& otherNode) const { return !(*this == otherNode); } diff --git a/libraries/networking/src/ReceivedPacketProcessor.h b/libraries/networking/src/ReceivedPacketProcessor.h index dd790a9b3d..4e4a3d1d11 100644 --- a/libraries/networking/src/ReceivedPacketProcessor.h +++ b/libraries/networking/src/ReceivedPacketProcessor.h @@ -20,6 +20,8 @@ class ReceivedPacketProcessor : public GenericThread { Q_OBJECT public: + static const unsigned long MAX_WAIT_TIME { 100 }; + ReceivedPacketProcessor(); /// Add packet from network receive thread to the processing queue. @@ -64,7 +66,7 @@ protected: virtual bool process() override; /// Determines the timeout of the wait when there are no packets to process. Default value means no timeout - virtual unsigned long getMaxWait() const { return ULONG_MAX; } + virtual unsigned long getMaxWait() const { return MAX_WAIT_TIME; } /// Override to do work before the packets processing loop. Default does nothing. virtual void preProcess() { } diff --git a/libraries/shared/src/GenericThread.cpp b/libraries/shared/src/GenericThread.cpp index 00a80a2864..2e126f12c9 100644 --- a/libraries/shared/src/GenericThread.cpp +++ b/libraries/shared/src/GenericThread.cpp @@ -10,6 +10,7 @@ // #include +#include #include "GenericThread.h" @@ -73,6 +74,7 @@ void GenericThread::threadRoutine() { } while (!_stopThread) { + QCoreApplication::processEvents(); // override this function to do whatever your class actually does, return false to exit thread early if (!process()) { From a15660993df065308a3fd39d391aa37ece315966 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Mon, 12 Jun 2017 10:54:22 -0700 Subject: [PATCH 2/4] Fix comment --- libraries/networking/src/ReceivedPacketProcessor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/ReceivedPacketProcessor.h b/libraries/networking/src/ReceivedPacketProcessor.h index 4e4a3d1d11..5b54d4f309 100644 --- a/libraries/networking/src/ReceivedPacketProcessor.h +++ b/libraries/networking/src/ReceivedPacketProcessor.h @@ -65,7 +65,7 @@ protected: /// Implements generic processing behavior for this thread. virtual bool process() override; - /// Determines the timeout of the wait when there are no packets to process. Default value means no timeout + /// Determines the timeout of the wait when there are no packets to process. Default value is 100ms to allow for regular event processing. virtual unsigned long getMaxWait() const { return MAX_WAIT_TIME; } /// Override to do work before the packets processing loop. Default does nothing. From 3cc23960217a93cb2403cda36b32dff286a4399d Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 13 Jun 2017 11:26:40 -0700 Subject: [PATCH 3/4] CR --- assignment-client/src/octree/OctreeInboundPacketProcessor.cpp | 2 +- assignment-client/src/octree/OctreeInboundPacketProcessor.h | 2 +- libraries/networking/src/ReceivedPacketProcessor.h | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp b/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp index d2fef4dfbd..04409b3b21 100644 --- a/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp +++ b/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp @@ -47,7 +47,7 @@ void OctreeInboundPacketProcessor::resetStats() { _singleSenderStats.clear(); } -unsigned long OctreeInboundPacketProcessor::getMaxWait() const { +uint32_t OctreeInboundPacketProcessor::getMaxWait() const { // calculate time until next sendNackPackets() quint64 nextNackTime = _lastNackTime + TOO_LONG_SINCE_LAST_NACK; quint64 now = usecTimestampNow(); diff --git a/assignment-client/src/octree/OctreeInboundPacketProcessor.h b/assignment-client/src/octree/OctreeInboundPacketProcessor.h index 4611fcada0..a7fa297d24 100644 --- a/assignment-client/src/octree/OctreeInboundPacketProcessor.h +++ b/assignment-client/src/octree/OctreeInboundPacketProcessor.h @@ -80,7 +80,7 @@ protected: virtual void processPacket(QSharedPointer message, SharedNodePointer sendingNode) override; - virtual unsigned long getMaxWait() const override; + virtual uint32_t getMaxWait() const override; virtual void preProcess() override; virtual void midProcess() override; diff --git a/libraries/networking/src/ReceivedPacketProcessor.h b/libraries/networking/src/ReceivedPacketProcessor.h index 5b54d4f309..f71abce1f1 100644 --- a/libraries/networking/src/ReceivedPacketProcessor.h +++ b/libraries/networking/src/ReceivedPacketProcessor.h @@ -20,7 +20,7 @@ class ReceivedPacketProcessor : public GenericThread { Q_OBJECT public: - static const unsigned long MAX_WAIT_TIME { 100 }; + static const uint64_t MAX_WAIT_TIME { 100 }; // Max wait time in ms ReceivedPacketProcessor(); @@ -66,7 +66,7 @@ protected: virtual bool process() override; /// Determines the timeout of the wait when there are no packets to process. Default value is 100ms to allow for regular event processing. - virtual unsigned long getMaxWait() const { return MAX_WAIT_TIME; } + virtual uint32_t getMaxWait() const { return MAX_WAIT_TIME; } /// Override to do work before the packets processing loop. Default does nothing. virtual void preProcess() { } From 5777b5e8050de9dcf22e0a3578df093274cae9f6 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Wed, 14 Jun 2017 10:53:36 -0700 Subject: [PATCH 4/4] Create one packet per node --- libraries/octree/src/JurisdictionListener.cpp | 3 +-- libraries/octree/src/JurisdictionSender.cpp | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/libraries/octree/src/JurisdictionListener.cpp b/libraries/octree/src/JurisdictionListener.cpp index dbbd146f4e..76c5069006 100644 --- a/libraries/octree/src/JurisdictionListener.cpp +++ b/libraries/octree/src/JurisdictionListener.cpp @@ -35,14 +35,13 @@ void JurisdictionListener::nodeKilled(SharedNodePointer node) { } bool JurisdictionListener::queueJurisdictionRequest() { - auto packet = NLPacket::create(PacketType::JurisdictionRequest, 0); - auto nodeList = DependencyManager::get(); int nodeCount = 0; nodeList->eachNode([&](const SharedNodePointer& node) { if (node->getType() == getNodeType() && node->getActiveSocket()) { + auto packet = NLPacket::create(PacketType::JurisdictionRequest, 0); _packetSender.queuePacketForSending(node, std::move(packet)); nodeCount++; } diff --git a/libraries/octree/src/JurisdictionSender.cpp b/libraries/octree/src/JurisdictionSender.cpp index ed3d59cebc..dfe1a6d872 100644 --- a/libraries/octree/src/JurisdictionSender.cpp +++ b/libraries/octree/src/JurisdictionSender.cpp @@ -41,8 +41,6 @@ bool JurisdictionSender::process() { // call our ReceivedPacketProcessor base class process so we'll get any pending packets if (continueProcessing && (continueProcessing = ReceivedPacketProcessor::process())) { - auto packet = (_jurisdictionMap) ? _jurisdictionMap->packIntoPacket() - : JurisdictionMap::packEmptyJurisdictionIntoMessage(getNodeType()); int nodeCount = 0; lockRequestingNodes(); @@ -53,6 +51,8 @@ bool JurisdictionSender::process() { SharedNodePointer node = DependencyManager::get()->nodeWithUUID(nodeUUID); if (node && node->getActiveSocket()) { + auto packet = (_jurisdictionMap) ? _jurisdictionMap->packIntoPacket() + : JurisdictionMap::packEmptyJurisdictionIntoMessage(getNodeType()); _packetSender.queuePacketForSending(node, std::move(packet)); nodeCount++; }