decouple STUN from DS check in

domain-server/src/DomainServer.cpp

@@ -354,37 +354,23 @@ bool DomainServer::optionallySetupAssignmentPayment() {
 void DomainServer::setupAutomaticNetworking() {
     auto nodeList = DependencyManager::get<LimitedNodeList>();
 
-    const int STUN_REFLEXIVE_KEEPALIVE_INTERVAL_MSECS = 10 * 1000;
-    const int STUN_IP_ADDRESS_CHECK_INTERVAL_MSECS = 30 * 1000;
-
-    // setup our timer to check our IP via stun every X seconds
-    QTimer* dynamicIPTimer = new QTimer(this);
-    connect(dynamicIPTimer, &QTimer::timeout, this, &DomainServer::requestCurrentPublicSocketViaSTUN);
-
     _automaticNetworkingSetting =
         _settingsManager.valueOrDefaultValueForKeyPath(METAVERSE_AUTOMATIC_NETWORKING_KEY_PATH).toString();
 
     if (_automaticNetworkingSetting == FULL_AUTOMATIC_NETWORKING_VALUE) {
-        dynamicIPTimer->start(STUN_REFLEXIVE_KEEPALIVE_INTERVAL_MSECS);
-
-        // setup a timer to heartbeat with the ice-server every so often
-        QTimer* iceHeartbeatTimer = new QTimer(this);
-        connect(iceHeartbeatTimer, &QTimer::timeout, this, &DomainServer::performICEUpdates);
-        iceHeartbeatTimer->start(ICE_HEARBEAT_INTERVAL_MSECS);
-
         // call our sendHeartbeatToIceServer immediately anytime a local or public socket changes
         connect(nodeList.data(), &LimitedNodeList::localSockAddrChanged,
                 this, &DomainServer::sendHeartbeatToIceServer);
         connect(nodeList.data(), &LimitedNodeList::publicSockAddrChanged,
                 this, &DomainServer::sendHeartbeatToIceServer);
 
-        // attempt to update our public socket now, this will send a heartbeat once we get public socket
+        // we need this DS to know what our public IP is - start trying to figure that out now
-        requestCurrentPublicSocketViaSTUN();
+        nodeList->startSTUNPublicSocketUpdate();
-
-        // in case the STUN lookup is still happening we should re-request a public socket once we get that address
-        connect(&nodeList->getSTUNSockAddr(), &HifiSockAddr::lookupCompleted,
-                this, &DomainServer::requestCurrentPublicSocketViaSTUN);
 
+        // setup a timer to heartbeat with the ice-server every so often
+        QTimer* iceHeartbeatTimer = new QTimer(this);
+        connect(iceHeartbeatTimer, &QTimer::timeout, this, &DomainServer::performICEUpdates);
+        iceHeartbeatTimer->start(ICE_HEARBEAT_INTERVAL_MSECS);
     }
 
     if (!didSetupAccountManagerWithAccessToken()) {
@@ -404,14 +390,12 @@ void DomainServer::setupAutomaticNetworking() {
                 << uuidStringWithoutCurlyBraces(domainID) << "via" << _oauthProviderURL.toString();
 
             if (_automaticNetworkingSetting == IP_ONLY_AUTOMATIC_NETWORKING_VALUE) {
-                dynamicIPTimer->start(STUN_IP_ADDRESS_CHECK_INTERVAL_MSECS);
+                // send any public socket changes to the data server so nodes can find us at our new IP
-
-                // send public socket changes to the data server so nodes can find us at our new IP
                 connect(nodeList.data(), &LimitedNodeList::publicSockAddrChanged,
                         this, &DomainServer::performIPAddressUpdate);
 
-                // attempt to update our sockets now
+                // have the LNL enable public socket updating via STUN
-                requestCurrentPublicSocketViaSTUN();
+                nodeList->startSTUNPublicSocketUpdate();
             } else {
                 // send our heartbeat to data server so it knows what our network settings are
                 sendHeartbeatToDataServer();
@@ -1216,10 +1200,6 @@ void DomainServer::transactionJSONCallback(const QJsonObject& data) {
     }
 }
 
-void DomainServer::requestCurrentPublicSocketViaSTUN() {
-    DependencyManager::get<LimitedNodeList>()->sendSTUNRequest();
-}
-
 QJsonObject jsonForDomainSocketUpdate(const HifiSockAddr& socket) {
     const QString SOCKET_NETWORK_ADDRESS_KEY = "network_address";
     const QString SOCKET_PORT_KEY = "port";

domain-server/src/DomainServer.h

@@ -62,7 +62,6 @@ private slots:
     void setupPendingAssignmentCredits();
     void sendPendingTransactionsToServer();
 
-    void requestCurrentPublicSocketViaSTUN();
     void performIPAddressUpdate(const HifiSockAddr& newPublicSockAddr);
     void performICEUpdates();
     void sendHeartbeatToDataServer() { sendHeartbeatToDataServer(QString()); }

libraries/networking/src/DomainHandler.cpp

@@ -181,7 +181,6 @@ void DomainHandler::completedHostnameLookup(const QHostInfo& hostInfo) {
 
             DependencyManager::get<NodeList>()->flagTimeForConnectionStep(NodeList::ConnectionStep::SetDomainSocket);
 
-
             qCDebug(networking, "DS at %s is at %s", _hostname.toLocal8Bit().constData(),
                    _sockAddr.getAddress().toString().toLocal8Bit().constData());
 

libraries/networking/src/LimitedNodeList.cpp

@@ -596,6 +596,22 @@ const int NUM_BYTES_STUN_HEADER = 20;
 
 void LimitedNodeList::sendSTUNRequest() {
 
+    static quint64 lastTimeStamp = usecTimestampNow();
+    lastTimeStamp = usecTimestampNow();
+
+    const int NUM_INITIAL_STUN_REQUESTS_BEFORE_FAIL = 10;
+
+    if (!_hasCompletedInitialSTUN) {
+        qCDebug(networking) << "Sending intial stun request to" << STUN_SERVER_HOSTNAME;
+
+        if (_numInitialSTUNRequests > NUM_INITIAL_STUN_REQUESTS_BEFORE_FAIL) {
+            // we're still trying to do our initial STUN we're over the fail threshold
+            stopInitialSTUNUpdate(false);
+        }
+
+        ++_numInitialSTUNRequests;
+    }
+
     unsigned char stunRequestPacket[NUM_BYTES_STUN_HEADER];
 
     int packetIndex = 0;
@@ -652,8 +668,6 @@ bool LimitedNodeList::processSTUNResponse(const QByteArray& packet) {
                 const int NUM_BYTES_FAMILY_ALIGN = 1;
                 const uint8_t IPV4_FAMILY_NETWORK_ORDER = htons(0x01) >> 8;
 
-
-
                 int byteIndex = attributeStartIndex + NUM_BYTES_STUN_ATTR_TYPE_AND_LENGTH + NUM_BYTES_FAMILY_ALIGN;
 
                 uint8_t addressFamily = 0;
@@ -679,6 +693,11 @@ bool LimitedNodeList::processSTUNResponse(const QByteArray& packet) {
                     QHostAddress newPublicAddress = QHostAddress(stunAddress);
 
                     if (newPublicAddress != _publicSockAddr.getAddress() || newPublicPort != _publicSockAddr.getPort()) {
+                        if (!_hasCompletedInitialSTUN) {
+                            // if we're here we have definitely completed our initial STUN sequence
+                            stopInitialSTUNUpdate(true);
+                        }
+
                         _publicSockAddr = HifiSockAddr(newPublicAddress, newPublicPort);
 
                         qCDebug(networking, "New public socket received from STUN server is %s:%hu",
@@ -707,6 +726,60 @@ bool LimitedNodeList::processSTUNResponse(const QByteArray& packet) {
     return false;
 }
 
+void LimitedNodeList::startSTUNPublicSocketUpdate() {
+    assert(!_initialSTUNTimer);
+
+    if (!_initialSTUNTimer) {
+        // if we don't know the STUN IP yet we need to have ourselves be called once it is known
+        if (_stunSockAddr.getAddress().isNull()) {
+            connect(&_stunSockAddr, &HifiSockAddr::lookupCompleted, this, &LimitedNodeList::startSTUNPublicSocketUpdate);
+        } else {
+            // setup our initial STUN timer here so we can quickly find out our public IP address
+            _initialSTUNTimer = new QTimer(this);
+
+            connect(_initialSTUNTimer.data(), &QTimer::timeout, this, &LimitedNodeList::sendSTUNRequest);
+
+            const int STUN_INITIAL_UPDATE_INTERVAL_MSECS = 250;
+           _initialSTUNTimer->start(STUN_INITIAL_UPDATE_INTERVAL_MSECS);
+        }
+    }
+}
+
+void LimitedNodeList::stopInitialSTUNUpdate(bool success) {
+    _hasCompletedInitialSTUN = true;
+
+    if (!success) {
+        // if we're here this was the last failed STUN request
+        // use our DS as our stun server
+        qCDebug(networking, "Failed to lookup public address via STUN server at %s:%hu.",
+                STUN_SERVER_HOSTNAME, STUN_SERVER_PORT);
+        qCDebug(networking) << "LimitedNodeList public socket will be set with local port and null QHostAddress.";
+
+        // reset the public address and port to a null address
+        _publicSockAddr = HifiSockAddr(QHostAddress(), _nodeSocket.localPort());
+
+        // we have changed the publicSockAddr, so emit our signal
+        emit publicSockAddrChanged(_publicSockAddr);
+    }
+
+    assert(_initialSTUNTimer);
+
+    // stop our initial fast timer
+    if (_initialSTUNTimer) {
+        _initialSTUNTimer->stop();
+        _initialSTUNTimer->deleteLater();
+    }
+
+    // We now setup a timer here to fire every so often to check that our IP address has not changed.
+    // Or, if we failed - if will check if we can eventually get a public socket
+    const int STUN_IP_ADDRESS_CHECK_INTERVAL_MSECS = 30 * 1000;
+
+    QTimer* stunOccasionalTimer = new QTimer(this);
+    connect(stunOccasionalTimer, &QTimer::timeout, this, &LimitedNodeList::sendSTUNRequest);
+
+    stunOccasionalTimer->start(STUN_IP_ADDRESS_CHECK_INTERVAL_MSECS);
+}
+
 void LimitedNodeList::updateLocalSockAddr() {
     HifiSockAddr newSockAddr(getLocalAddress(), _nodeSocket.localPort());
     if (newSockAddr != _localSockAddr) {

libraries/networking/src/LimitedNodeList.h

@@ -21,13 +21,14 @@
 #include <unistd.h> // not on windows, not needed for mac or windows
 #endif
 
-#include <qelapsedtimer.h>
+#include <QtCore/QElapsedTimer>
-#include <qreadwritelock.h>
+#include <QtCore/QPointer>
-#include <qset.h>
+#include <QtCore/QReadWriteLock>
-#include <qsharedpointer.h>
+#include <QtCore/QSet>
-#include <QtNetwork/qudpsocket.h>
+#include <QtCore/QSharedMemory>
-#include <QtNetwork/qhostaddress.h>
+#include <QtCore/QSharedPointer>
-#include <QSharedMemory>
+#include <QtNetwork/QUdpSocket>
+#include <QtNetwork/QHostAddress>
 
 #include <tbb/concurrent_unordered_map.h>
 
@@ -130,6 +131,8 @@ public:
                                       const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket,
                                       bool canAdjustLocks, bool canRez);
 
+    bool hasCompletedInitialSTUN() const { return _hasCompletedInitialSTUN; }
+
     const HifiSockAddr& getLocalSockAddr() const { return _localSockAddr; }
     const HifiSockAddr& getSTUNSockAddr() const { return _stunSockAddr; }
 
@@ -149,7 +152,6 @@ public:
                                    const QUuid& packetHeaderID = QUuid());
     QByteArray constructPingReplyPacket(const QByteArray& pingPacket, const QUuid& packetHeaderID = QUuid());
 
-    virtual void sendSTUNRequest();
     virtual bool processSTUNResponse(const QByteArray& packet);
 
     void sendHeartbeatToIceServer(const HifiSockAddr& iceServerSockAddr,
@@ -210,6 +212,9 @@ public slots:
 
     void updateLocalSockAddr();
 
+    void startSTUNPublicSocketUpdate();
+    virtual void sendSTUNRequest();
+
     void killNodeWithUUID(const QUuid& nodeUUID);
 signals:
     void uuidChanged(const QUuid& ownerUUID, const QUuid& oldUUID);
@@ -240,6 +245,8 @@ protected:
 
     void handleNodeKill(const SharedNodePointer& node);
 
+    void stopInitialSTUNUpdate(bool success);
+
     QUuid _sessionUUID;
     NodeHash _nodeHash;
     QReadWriteLock _nodeMutex;
@@ -259,6 +266,10 @@ protected:
 
     std::unordered_map<QUuid, PacketTypeSequenceMap, UUIDHasher> _packetSequenceNumbers;
 
+    QPointer<QTimer> _initialSTUNTimer;
+    int _numInitialSTUNRequests = 0;
+    bool _hasCompletedInitialSTUN = false;
+
     template<typename IteratorLambda>
     void eachNodeHashIterator(IteratorLambda functor) {
         QWriteLocker writeLock(&_nodeMutex);
@@ -268,7 +279,6 @@ protected:
             functor(it);
         }
     }
-
 };
 
 #endif // hifi_LimitedNodeList_h

libraries/networking/src/NodeList.cpp

@@ -36,9 +36,7 @@ NodeList::NodeList(char newOwnerType, unsigned short socketListenPort, unsigned
     _nodeTypesOfInterest(),
     _domainHandler(this),
     _numNoReplyDomainCheckIns(0),
-    _assignmentServerSocket(),
+    _assignmentServerSocket()
-    _hasCompletedInitialSTUNFailure(false),
-    _stunRequestsSinceSuccess(0)
 {
     static bool firstCall = true;
     if (firstCall) {
@@ -63,6 +61,12 @@ NodeList::NodeList(char newOwnerType, unsigned short socketListenPort, unsigned
     // fire off any pending DS path query when we get socket information
     connect(&_domainHandler, &DomainHandler::completedSocketDiscovery, this, &NodeList::sendPendingDSPathQuery);
 
+    // send a domain server check in immediately once the DS socket is known
+    connect(&_domainHandler, &DomainHandler::completedSocketDiscovery, this, &NodeList::sendDomainServerCheckIn);
+
+    // send a domain server check in immediately if there is a public socket change
+    connect(this, &LimitedNodeList::publicSockAddrChanged, this, &NodeList::sendDomainServerCheckIn);
+
     // clear our NodeList when the domain changes
     connect(&_domainHandler, &DomainHandler::disconnectedFromDomain, this, &NodeList::reset);
 
@@ -79,6 +83,9 @@ NodeList::NodeList(char newOwnerType, unsigned short socketListenPort, unsigned
     connect(&AccountManager::getInstance(), &AccountManager::logoutComplete , this, &NodeList::reset);
 
     qRegisterMetaType<NodeList::ConnectionStep>("NodeList::ConnectionStep");
+
+    // we definitely want STUN to update our public socket, so call the LNL to kick that off
+    startSTUNPublicSocketUpdate();
 }
 
 qint64 NodeList::sendStats(const QJsonObject& statsObject, const HifiSockAddr& destination) {
@@ -233,9 +240,6 @@ void NodeList::processNodeData(const HifiSockAddr& senderSockAddr, const QByteAr
             } else {
                 qCDebug(networking) << "Reply does not match either local or public socket for domain. Will not connect.";
             }
-
-            // immediately send a domain-server check in now that we have channel to talk to domain-server on
-            sendDomainServerCheckIn();
         }
         case PacketTypeStunResponse: {
             // a STUN packet begins with 00, we've checked the second zero with packetVersionMatch
@@ -280,57 +284,13 @@ void NodeList::addSetOfNodeTypesToNodeInterestSet(const NodeSet& setOfNodeTypes)
     _nodeTypesOfInterest.unite(setOfNodeTypes);
 }
 
-
-const unsigned int NUM_STUN_REQUESTS_BEFORE_FALLBACK = 5;
-
-void NodeList::sendSTUNRequest() {
-
-    if (!_hasCompletedInitialSTUNFailure) {
-        qCDebug(networking) << "Sending intial stun request to" << STUN_SERVER_HOSTNAME;
-    }
-
-    LimitedNodeList::sendSTUNRequest();
-
-    _stunRequestsSinceSuccess++;
-
-    if (_stunRequestsSinceSuccess >= NUM_STUN_REQUESTS_BEFORE_FALLBACK) {
-        if (!_hasCompletedInitialSTUNFailure) {
-            // if we're here this was the last failed STUN request
-            // use our DS as our stun server
-            qCDebug(networking, "Failed to lookup public address via STUN server at %s:%hu. Using DS for STUN.",
-                   STUN_SERVER_HOSTNAME, STUN_SERVER_PORT);
-
-            _hasCompletedInitialSTUNFailure = true;
-        }
-
-        // reset the public address and port
-        // use 0 so the DS knows to act as out STUN server
-        _publicSockAddr = HifiSockAddr(QHostAddress(), _nodeSocket.localPort());
-    }
-}
-
-bool NodeList::processSTUNResponse(const QByteArray& packet) {
-    if (LimitedNodeList::processSTUNResponse(packet)) {
-        // reset the number of failed STUN requests since last success
-        _stunRequestsSinceSuccess = 0;
-
-        _hasCompletedInitialSTUNFailure = true;
-
-        return true;
-    } else {
-        return false;
-    }
-}
-
 void NodeList::sendDomainServerCheckIn() {
-    if (_publicSockAddr.isNull() && !_hasCompletedInitialSTUNFailure) {
+    if (_publicSockAddr.isNull()) {
         // we don't know our public socket and we need to send it to the domain server
-        // send a STUN request to figure it out
+        qCDebug(networking) << "Waiting for inital public socket from STUN. Will not send domain-server check in.";
-        sendSTUNRequest();
     } else if (_domainHandler.getIP().isNull() && _domainHandler.requiresICE()) {
         handleICEConnectionToDomainServer();
     } else if (!_domainHandler.getIP().isNull()) {
-
         bool isUsingDTLS = false;
 
         PacketType domainPacketType = !_domainHandler.isConnected()
@@ -375,7 +335,6 @@ void NodeList::sendDomainServerCheckIn() {
         // pack our data to send to the domain-server
         packetStream << _ownerType << _publicSockAddr << _localSockAddr << _nodeTypesOfInterest.toList();
 
-
         // if this is a connect request, and we can present a username signature, send it along
         if (!_domainHandler.isConnected()) {
             DataServerAccountInfo& accountInfo = AccountManager::getInstance().getAccountInfo();
@@ -395,14 +354,6 @@ void NodeList::sendDomainServerCheckIn() {
             writeUnverifiedDatagram(domainServerPacket, _domainHandler.getSockAddr());
         }
 
-        const int NUM_DOMAIN_SERVER_CHECKINS_PER_STUN_REQUEST = 5;
-        static unsigned int numDomainCheckins = 0;
-
-        // send a STUN request every Nth domain server check in so we update our public socket, if required
-        if (numDomainCheckins++ % NUM_DOMAIN_SERVER_CHECKINS_PER_STUN_REQUEST == 0) {
-            sendSTUNRequest();
-        }
-
         if (_numNoReplyDomainCheckIns >= MAX_SILENT_DOMAIN_SERVER_CHECK_INS) {
             // we haven't heard back from DS in MAX_SILENT_DOMAIN_SERVER_CHECK_INS
             // so emit our signal that says that

libraries/networking/src/NodeList.h

@@ -56,6 +56,7 @@ public:
         SendFirstPingsToDS,
         SetDomainHostname,
         SetDomainSocket,
+        ForcedSTUNRequest,
         SendFirstDSCheckIn,
         ReceiveFirstDSList,
         SendFirstAudioPing,
@@ -110,9 +111,6 @@ private:
     NodeList(NodeList const&); // Don't implement, needed to avoid copies of singleton
     void operator=(NodeList const&); // Don't implement, needed to avoid copies of singleton
 
-    void sendSTUNRequest();
-    bool processSTUNResponse(const QByteArray& packet);
-
     void processDomainServerAuthRequest(const QByteArray& packet);
     void requestAuthForDomainServer();
     void activateSocketFromNodeCommunication(const QByteArray& packet, const SharedNodePointer& sendingNode);
@@ -127,8 +125,6 @@ private:
     DomainHandler _domainHandler;
     int _numNoReplyDomainCheckIns;
     HifiSockAddr _assignmentServerSocket;
-    bool _hasCompletedInitialSTUNFailure;
-    unsigned int _stunRequestsSinceSuccess;
 
     mutable QReadWriteLock _connectionTimeLock { };
     QMap<ConnectionStep, quint64> _lastConnectionTimes;