fix memory issues for immediate ice pings

domain-server/src/DomainServer.cpp

@@ -369,7 +369,7 @@ void DomainServer::setupAutomaticNetworking() {
 
         // setup a timer to heartbeat with the ice-server every so often
         QTimer* iceHeartbeatTimer = new QTimer(this);
-        connect(iceHeartbeatTimer, &QTimer::timeout, this, &DomainServer::performICEUpdates);
+        connect(iceHeartbeatTimer, &QTimer::timeout, this, &DomainServer::sendHeartbeatToIceServer);
         iceHeartbeatTimer->start(ICE_HEARBEAT_INTERVAL_MSECS);
     }
 
@@ -624,9 +624,17 @@ void DomainServer::handleConnectRequest(const QByteArray& packet, const HifiSock
 
         QUuid nodeUUID;
 
-        if (_connectingICEPeers.contains(packetUUID) || _connectedICEPeers.contains(packetUUID)) {
+        HifiSockAddr discoveredSocket = senderSockAddr;
+        SharedNetworkPeer connectedPeer = _icePeers.value(packetUUID);
+
+        if (connectedPeer) {
             //  this user negotiated a connection with us via ICE, so re-use their ICE client ID
             nodeUUID = packetUUID;
+
+            if (connectedPeer->getActiveSocket()) {
+                // set their discovered socket to whatever the activated socket on the network peer object was
+                discoveredSocket = *connectedPeer->getActiveSocket();
+            }
         } else {
             // we got a packetUUID we didn't recognize, just add the node
             nodeUUID = QUuid::createUuid();
@@ -656,7 +664,7 @@ void DomainServer::handleConnectRequest(const QByteArray& packet, const HifiSock
                                                                      canAdjustLocks, canRez);
 
         // So that we can send messages to this node at will - we need to activate the correct socket on this node now
-        newNode->activateMatchingOrNewSymmetricSocket(senderSockAddr);
+        newNode->activateMatchingOrNewSymmetricSocket(discoveredSocket);
 
         // when the newNode is created the linked data is also created
         // if this was a static assignment set the UUID, set the sendingSockAddr
@@ -693,7 +701,6 @@ void DomainServer::handleConnectRequest(const QByteArray& packet, const HifiSock
     }
 }
 
-
 unsigned int DomainServer::countConnectedUsers() {
     unsigned int result = 0;
     auto nodeList = DependencyManager::get<LimitedNodeList>();
@@ -936,13 +943,6 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif
 
     DomainServerNodeData* nodeData = reinterpret_cast<DomainServerNodeData*>(node->getLinkedData());
 
-    // if we've established a connection via ICE with this peer, use that socket
-    // otherwise just try to reply back to them on their sending socket (although that may not work)
-    HifiSockAddr destinationSockAddr = _connectedICEPeers.value(node->getUUID());
-    if (destinationSockAddr.isNull()) {
-        destinationSockAddr = senderSockAddr;
-    }
-
     if (nodeInterestList.size() > 0) {
 
 //        DTLSServerSession* dtlsSession = _isUsingDTLS ? _dtlsSessions[senderSockAddr] : NULL;
@@ -982,7 +982,7 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif
     }
 
     // always write the last broadcastPacket
-    limitedNodeList->writeUnverifiedDatagram(broadcastPacket, node, senderSockAddr);
+    limitedNodeList->writeUnverifiedDatagram(broadcastPacket, node);
 }
 
 QUuid DomainServer::connectionSecretForNodes(const SharedNodePointer& nodeA, const SharedNodePointer& nodeB) {
@@ -1306,45 +1306,45 @@ void DomainServer::sendHeartbeatToDataServer(const QString& networkAddress) {
                                               domainUpdateJSON.toUtf8());
 }
 
-// todo: have data-web respond with ice-server hostname to use
+// TODO: have data-web respond with ice-server hostname to use
-
-void DomainServer::performICEUpdates() {
-    sendHeartbeatToIceServer();
-    sendICEPingPackets();
-}
 
 void DomainServer::sendHeartbeatToIceServer() {
     DependencyManager::get<LimitedNodeList>()->sendHeartbeatToIceServer(_iceServerSocket);
 }
 
-void DomainServer::sendICEPingPackets() {
+const int NUM_PEER_PINGS_BEFORE_DELETE = 2000 / UDP_PUNCH_PING_INTERVAL_MS;
-    auto nodeList = DependencyManager::get<LimitedNodeList>();
 
-    QHash<QUuid, NetworkPeer>::iterator peer = _connectingICEPeers.begin();
+void DomainServer::pingPunchForConnectingPeer(const SharedNetworkPeer& peer) {
 
-    while (peer != _connectingICEPeers.end()) {
+    if (peer->getConnectionAttempts() > 0 && peer->getConnectionAttempts() % NUM_PEER_PINGS_BEFORE_DELETE == 0) {
+        // we've reached the maximum number of ping attempts
+        qDebug() << "Maximum number of ping attempts reached for peer with ID" << peer->getUUID();
+        qDebug() << "Removing from list of connecting peers.";
 
-        if (peer->getConnectionAttempts() >= MAX_ICE_CONNECTION_ATTEMPTS) {
+        _icePeers.remove(peer->getUUID());
-            // we've already tried to connect to this peer enough times
+    } else {
-            // remove it from our list - if it wants to re-connect it'll come back through ice-server
+        auto nodeList = DependencyManager::get<LimitedNodeList>();
-            peer = _connectingICEPeers.erase(peer);
-        } else {
-            // send ping packets to this peer's interfaces
-            qDebug() << "Sending ping packets to establish connectivity with ICE peer with ID"
-                << peer->getUUID();
 
-            // send the ping packet to the local and public sockets for this node
+        // send ping packets to this peer's interfaces
-            QByteArray localPingPacket = nodeList->constructPingPacket(PingType::Local, false);
+        qDebug() << "Sending ping packets to establish connectivity with ICE peer with ID"
-            nodeList->writeUnverifiedDatagram(localPingPacket, peer->getLocalSocket());
+            << peer->getUUID();
 
-            QByteArray publicPingPacket = nodeList->constructPingPacket(PingType::Public, false);
+        // send the ping packet to the local and public sockets for this node
-            nodeList->writeUnverifiedDatagram(publicPingPacket, peer->getPublicSocket());
+        QByteArray localPingPacket = nodeList->constructPingPacket(PingType::Local, false);
+        nodeList->writeUnverifiedDatagram(localPingPacket, peer->getLocalSocket());
 
-            peer->incrementConnectionAttempts();
+        QByteArray publicPingPacket = nodeList->constructPingPacket(PingType::Public, false);
+        nodeList->writeUnverifiedDatagram(publicPingPacket, peer->getPublicSocket());
 
-            // go to next peer in hash
+        peer->incrementConnectionAttempts();
-            ++peer;
+    }
-        }
+}
+
+void DomainServer::handlePeerPingTimeout() {
+    SharedNetworkPeer senderPeer = _icePeers.value(qobject_cast<NetworkPeer*>(sender())->getUUID());
+
+    if (senderPeer && !senderPeer->getActiveSocket()) {
+        pingPunchForConnectingPeer(senderPeer);
     }
 }
 
@@ -1354,31 +1354,32 @@ void DomainServer::processICEPeerInformation(const QByteArray& packet) {
     QDataStream iceResponseStream(packet);
     iceResponseStream.skipRawData(numBytesForPacketHeader(packet));
 
-    NetworkPeer receivedPeer;
+    NetworkPeer* receivedPeer = new NetworkPeer;
-    iceResponseStream >> receivedPeer;
+    iceResponseStream >> *receivedPeer;
 
-    if (!_connectedICEPeers.contains(receivedPeer.getUUID())) {
+    if (!_icePeers.contains(receivedPeer->getUUID())) {
-        if (!_connectingICEPeers.contains(receivedPeer.getUUID())) {
+        qDebug() << "New peer requesting ICE connection being added to hash -" << *receivedPeer;
-            qDebug() << "New peer requesting connection being added to hash -" << receivedPeer;
+        SharedNetworkPeer newPeer = SharedNetworkPeer(receivedPeer);
-        }
+        _icePeers[receivedPeer->getUUID()] = newPeer;
 
-        _connectingICEPeers[receivedPeer.getUUID()] = receivedPeer;
+        // make sure we know when we should ping this peer
+        connect(newPeer.data(), &NetworkPeer::pingTimerTimeout, this, &DomainServer::handlePeerPingTimeout);
+
+        // immediately ping the new peer, and start a timer to continue pinging it until we connect to it
+        newPeer->startPingTimer();
+        pingPunchForConnectingPeer(newPeer);
+    } else {
+        delete receivedPeer;
     }
 }
 
 void DomainServer::processICEPingReply(const QByteArray& packet, const HifiSockAddr& senderSockAddr) {
     QUuid nodeUUID = uuidFromPacketHeader(packet);
-    NetworkPeer sendingPeer = _connectingICEPeers.take(nodeUUID);
+    SharedNetworkPeer sendingPeer = _icePeers.value(nodeUUID);
 
-    if (!sendingPeer.isNull()) {
+    if (sendingPeer) {
         // we had this NetworkPeer in our connecting list - add the right sock addr to our connected list
-        if (senderSockAddr == sendingPeer.getLocalSocket()) {
+        sendingPeer->activateMatchingOrNewSymmetricSocket(senderSockAddr);
-            qDebug() << "Activating local socket for communication with network peer -" << sendingPeer;
-            _connectedICEPeers.insert(nodeUUID, sendingPeer.getLocalSocket());
-        } else if (senderSockAddr == sendingPeer.getPublicSocket()) {
-            qDebug() << "Activating public socket for communication with network peer -" << sendingPeer;
-            _connectedICEPeers.insert(nodeUUID, sendingPeer.getPublicSocket());
-        }
     }
 }
 
@@ -2106,9 +2107,8 @@ void DomainServer::nodeAdded(SharedNodePointer node) {
 
 void DomainServer::nodeKilled(SharedNodePointer node) {
 
-    // remove this node from the connecting / connected ICE lists (if they exist)
+    // if this peer connected via ICE then remove them from our ICE peers hash
-    _connectingICEPeers.remove(node->getUUID());
+    _icePeers.remove(node->getUUID());
-    _connectedICEPeers.remove(node->getUUID());
 
     DomainServerNodeData* nodeData = reinterpret_cast<DomainServerNodeData*>(node->getLinkedData());
 

domain-server/src/DomainServer.h

@@ -63,10 +63,9 @@ private slots:
     void sendPendingTransactionsToServer();
 
     void performIPAddressUpdate(const HifiSockAddr& newPublicSockAddr);
-    void performICEUpdates();
     void sendHeartbeatToDataServer() { sendHeartbeatToDataServer(QString()); }
     void sendHeartbeatToIceServer();
-    void sendICEPingPackets();
+    void handlePeerPingTimeout();
 private:
     void setupNodeListAndAssignments(const QUuid& sessionUUID = QUuid::createUuid());
     bool optionallySetupOAuth();
@@ -81,6 +80,8 @@ private:
     void processICEPingReply(const QByteArray& packet, const HifiSockAddr& senderSockAddr);
     void processICEPeerInformation(const QByteArray& packet);
 
+    void pingPunchForConnectingPeer(const SharedNetworkPeer& peer);
+
     void processDatagram(const QByteArray& receivedPacket, const HifiSockAddr& senderSockAddr);
 
     void handleConnectRequest(const QByteArray& packet, const HifiSockAddr& senderSockAddr);
@@ -153,8 +154,7 @@ private:
 
     QHash<QString, QByteArray> _userPublicKeys;
 
-    QHash<QUuid, NetworkPeer> _connectingICEPeers;
+    QHash<QUuid, SharedNetworkPeer> _icePeers;
-    QHash<QUuid, HifiSockAddr> _connectedICEPeers;
 
     QString _automaticNetworkingSetting;
 

libraries/networking/src/DomainHandler.cpp

@@ -43,7 +43,7 @@ DomainHandler::DomainHandler(QObject* parent) :
 void DomainHandler::clearConnectionInfo() {
     _uuid = QUuid();
 
-    _icePeer = NetworkPeer();
+    _icePeer.reset();
 
     if (requiresICE()) {
         // if we connected to this domain with ICE, re-set the socket so we reconnect through the ice-server
@@ -299,16 +299,15 @@ void DomainHandler::processICEResponsePacket(const QByteArray& icePacket) {
     QDataStream iceResponseStream(icePacket);
     iceResponseStream.skipRawData(numBytesForPacketHeader(icePacket));
 
-    NetworkPeer packetPeer;
+    iceResponseStream >> _icePeer;
-    iceResponseStream >> packetPeer;
 
     DependencyManager::get<NodeList>()->flagTimeForConnectionStep(LimitedNodeList::ConnectionStep::ReceiveDSPeerInformation);
 
-    if (packetPeer.getUUID() != _iceDomainID) {
+    if (_icePeer.getUUID() != _iceDomainID) {
         qCDebug(networking) << "Received a network peer with ID that does not match current domain. Will not attempt connection.";
+        _icePeer.reset();
     } else {
-        qCDebug(networking) << "Received network peer object for domain -" << packetPeer;
+        qCDebug(networking) << "Received network peer object for domain -" << _icePeer;
-        _icePeer = packetPeer;
 
         // ask the peer object to start its ping timer
         _icePeer.startPingTimer();

libraries/networking/src/NetworkPeer.cpp

@@ -49,44 +49,8 @@ NetworkPeer::NetworkPeer(const QUuid& uuid, const HifiSockAddr& publicSocket, co
 
 }
 
-NetworkPeer::NetworkPeer(const NetworkPeer& otherPeer) : QObject() {
+NetworkPeer::~NetworkPeer() {
-    _uuid = otherPeer._uuid;
+    qDebug() << "Removing network peer with ID" << _uuid;
-    _publicSocket = otherPeer._publicSocket;
-    _localSocket = otherPeer._localSocket;
-    _symmetricSocket = otherPeer._symmetricSocket;
-
-    if (otherPeer._activeSocket) {
-        if (otherPeer._activeSocket == &otherPeer._localSocket) {
-            _activeSocket = &_localSocket;
-        } else if (otherPeer._activeSocket == &otherPeer._publicSocket) {
-            _activeSocket = &_publicSocket;
-        } else if (otherPeer._activeSocket == &otherPeer._symmetricSocket) {
-            _activeSocket = &_symmetricSocket;
-        }
-    }
-
-    _wakeTimestamp = otherPeer._wakeTimestamp;
-    _lastHeardMicrostamp = otherPeer._lastHeardMicrostamp;
-    _connectionAttempts = otherPeer._connectionAttempts;
-}
-
-NetworkPeer& NetworkPeer::operator=(const NetworkPeer& otherPeer) {
-    NetworkPeer temp(otherPeer);
-    swap(temp);
-    return *this;
-}
-
-void NetworkPeer::swap(NetworkPeer& otherPeer) {
-    using std::swap;
-
-    swap(_uuid, otherPeer._uuid);
-    swap(_publicSocket, otherPeer._publicSocket);
-    swap(_localSocket, otherPeer._localSocket);
-    swap(_symmetricSocket, otherPeer._symmetricSocket);
-    swap(_activeSocket, otherPeer._activeSocket);
-    swap(_wakeTimestamp, otherPeer._wakeTimestamp);
-    swap(_lastHeardMicrostamp, otherPeer._lastHeardMicrostamp);
-    swap(_connectionAttempts, otherPeer._connectionAttempts);
 }
 
 void NetworkPeer::setPublicSocket(const HifiSockAddr& publicSocket) {
@@ -181,6 +145,8 @@ void NetworkPeer::softReset() {
     // a soft reset should clear the sockets and reset the number of connection attempts
     _localSocket.clear();
     _publicSocket.clear();
+    _symmetricSocket.clear();
+    _activeSocket = NULL;
 
     // stop our ping timer since we don't have sockets to ping anymore anyways
     stopPingTimer();
@@ -188,6 +154,14 @@ void NetworkPeer::softReset() {
     _connectionAttempts = 0;
 }
 
+void NetworkPeer::reset() {
+    softReset();
+
+    _uuid = QUuid();
+    _wakeTimestamp = QDateTime::currentMSecsSinceEpoch();
+    _lastHeardMicrostamp = usecTimestampNow();
+}
+
 QByteArray NetworkPeer::toByteArray() const {
     QByteArray peerByteArray;
 

libraries/networking/src/NetworkPeer.h

@@ -31,8 +31,7 @@ public:
     NetworkPeer(QObject* parent = 0);
     NetworkPeer(const QUuid& uuid, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, QObject* parent = 0);
 
-    NetworkPeer(const NetworkPeer &otherPeer);
+    ~NetworkPeer();
-    NetworkPeer& operator=(const NetworkPeer& otherPeer);
 
     bool isNull() const { return _uuid.isNull(); }
     bool hasSockets() const { return !_localSocket.isNull() && !_publicSocket.isNull(); }
@@ -41,6 +40,7 @@ public:
     void setUUID(const QUuid& uuid) { _uuid = uuid; }
 
     void softReset();
+    void reset();
 
     const HifiSockAddr& getPublicSocket() const { return _publicSocket; }
     const HifiSockAddr& getLocalSocket() const { return _localSocket; }
@@ -99,8 +99,6 @@ protected:
     QTimer* _pingTimer = NULL;
 
     int _connectionAttempts;
-private:
-    void swap(NetworkPeer& otherPeer);
 };
 
 QDebug operator<<(QDebug debug, const NetworkPeer &peer);

libraries/networking/src/NodeList.cpp

@@ -193,7 +193,9 @@ void NodeList::processNodeData(const HifiSockAddr& senderSockAddr, const QByteAr
             break;
         }
         case PacketTypeIceServerPeerInformation: {
-            _domainHandler.processICEResponsePacket(packet);
+            if (!_domainHandler.getICEPeer().hasSockets()) {
+                _domainHandler.processICEResponsePacket(packet);
+            }
             break;
         }
         case PacketTypePing: {