From 9d2e1773a09dc2cf9306585cd6da93dc16b06a8c Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 28 May 2015 15:31:21 -0700 Subject: [PATCH] fix memory issues for immediate ice pings --- domain-server/src/DomainServer.cpp | 116 ++++++++++----------- domain-server/src/DomainServer.h | 8 +- libraries/networking/src/DomainHandler.cpp | 11 +- libraries/networking/src/NetworkPeer.cpp | 50 +++------ libraries/networking/src/NetworkPeer.h | 6 +- libraries/networking/src/NodeList.cpp | 4 +- 6 files changed, 84 insertions(+), 111 deletions(-) diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 0ffdabcb8a..7827271aa9 100644 --- a/domain-server/src/DomainServer.cpp +++ b/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(); @@ -936,13 +943,6 @@ void DomainServer::sendDomainListToNode(const SharedNodePointer& node, const Hif DomainServerNodeData* nodeData = reinterpret_cast(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 - -void DomainServer::performICEUpdates() { - sendHeartbeatToIceServer(); - sendICEPingPackets(); -} +// TODO: have data-web respond with ice-server hostname to use void DomainServer::sendHeartbeatToIceServer() { DependencyManager::get()->sendHeartbeatToIceServer(_iceServerSocket); } -void DomainServer::sendICEPingPackets() { - auto nodeList = DependencyManager::get(); +const int NUM_PEER_PINGS_BEFORE_DELETE = 2000 / UDP_PUNCH_PING_INTERVAL_MS; - QHash::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) { - // we've already tried to connect to this peer enough times - // remove it from our list - if it wants to re-connect it'll come back through ice-server - 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(); + _icePeers.remove(peer->getUUID()); + } else { + auto nodeList = DependencyManager::get(); - // send the ping packet to the local and public sockets for this node - QByteArray localPingPacket = nodeList->constructPingPacket(PingType::Local, false); - nodeList->writeUnverifiedDatagram(localPingPacket, peer->getLocalSocket()); + // send ping packets to this peer's interfaces + qDebug() << "Sending ping packets to establish connectivity with ICE peer with ID" + << peer->getUUID(); - QByteArray publicPingPacket = nodeList->constructPingPacket(PingType::Public, false); - nodeList->writeUnverifiedDatagram(publicPingPacket, peer->getPublicSocket()); + // send the ping packet to the local and public sockets for this node + 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; - } + peer->incrementConnectionAttempts(); + } +} + +void DomainServer::handlePeerPingTimeout() { + SharedNetworkPeer senderPeer = _icePeers.value(qobject_cast(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; - iceResponseStream >> receivedPeer; + NetworkPeer* receivedPeer = new NetworkPeer; + iceResponseStream >> *receivedPeer; - if (!_connectedICEPeers.contains(receivedPeer.getUUID())) { - if (!_connectingICEPeers.contains(receivedPeer.getUUID())) { - qDebug() << "New peer requesting connection being added to hash -" << receivedPeer; - } + if (!_icePeers.contains(receivedPeer->getUUID())) { + qDebug() << "New peer requesting ICE 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()) { - 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()); - } + sendingPeer->activateMatchingOrNewSymmetricSocket(senderSockAddr); } } @@ -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) - _connectingICEPeers.remove(node->getUUID()); - _connectedICEPeers.remove(node->getUUID()); + // if this peer connected via ICE then remove them from our ICE peers hash + _icePeers.remove(node->getUUID()); DomainServerNodeData* nodeData = reinterpret_cast(node->getLinkedData()); diff --git a/domain-server/src/DomainServer.h b/domain-server/src/DomainServer.h index 302f683108..7a9fb2fe9b 100644 --- a/domain-server/src/DomainServer.h +++ b/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 _userPublicKeys; - QHash _connectingICEPeers; - QHash _connectedICEPeers; + QHash _icePeers; QString _automaticNetworkingSetting; diff --git a/libraries/networking/src/DomainHandler.cpp b/libraries/networking/src/DomainHandler.cpp index e09dcb186b..38d1ade2ad 100644 --- a/libraries/networking/src/DomainHandler.cpp +++ b/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 >> packetPeer; + iceResponseStream >> _icePeer; DependencyManager::get()->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; - _icePeer = packetPeer; + qCDebug(networking) << "Received network peer object for domain -" << _icePeer; // ask the peer object to start its ping timer _icePeer.startPingTimer(); diff --git a/libraries/networking/src/NetworkPeer.cpp b/libraries/networking/src/NetworkPeer.cpp index f1197e5da0..83e5e72a87 100644 --- a/libraries/networking/src/NetworkPeer.cpp +++ b/libraries/networking/src/NetworkPeer.cpp @@ -49,44 +49,8 @@ NetworkPeer::NetworkPeer(const QUuid& uuid, const HifiSockAddr& publicSocket, co } -NetworkPeer::NetworkPeer(const NetworkPeer& otherPeer) : QObject() { - _uuid = otherPeer._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); +NetworkPeer::~NetworkPeer() { + qDebug() << "Removing network peer with ID" << _uuid; } 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; diff --git a/libraries/networking/src/NetworkPeer.h b/libraries/networking/src/NetworkPeer.h index a4db99ba9e..8cbb6cbe24 100644 --- a/libraries/networking/src/NetworkPeer.h +++ b/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& operator=(const NetworkPeer& otherPeer); + ~NetworkPeer(); 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); diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index dc6b33d0a9..326573ef35 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/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: {