ICE client: fix retrigger

Sometimes the RFC algorithm wants you to immediately 'retrigger' a
candidate check.  The previous code was deleting the previous request
and creating a new one.  But this means that any reply that comes in
cannot get matched up, because the new request will get a new
transaction ID.  In general, I think any time we cancel a request and
throw it away, it is probably a mistake, because the reply that comes
back contains useful information, and if we throw away the request, we
won't be able to match it up by transaction ID and remember what we were
doing, so we lose that information.

This can be catastrophic.  The Mac CI was failing because of a timing
issue where each side would 'retrigger' its request just before the
reply to the previous request arrived.  All of the responses were
getting dropped because they could not be matched up by transaction ID.

The new code just resets the retransmission timer, causing us to send it
immediately and also resets the exponential backoff schedule.

Also, simplify some logic and tweak formatting.

Author: zpostfacto

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp

@@ -895,6 +895,12 @@ void CSteamNetworkingSocketsSTUNRequest::Cancel()
     delete this;
 }
 
+void CSteamNetworkingSocketsSTUNRequest::RetriggerNow()
+{
+    m_nRetryCount = 0;
+    SetNextThinkTimeASAP();
+}
+
 void CSteamNetworkingSocketsSTUNRequest::Think( SteamNetworkingMicroseconds usecNow )
 {
 	SteamNetworkingGlobalLock::AssertHeldByCurrentThread( "CSteamNetworkingSocketsSTUNRequest::Think" );
@@ -1627,50 +1633,67 @@ void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, IC
     // An incoming request proves the remote side can reach us, so retry immediately
     // rather than waiting for the normal schedule or a retransmit timeout.
     // Skip when USE-CANDIDATE is set — that path handles its own triggered check below.
-    if ( !FindAttributeOfType( vecAttrs.Base(), vecAttrs.Count(), k_nSTUN_Attr_UseCandidate )
-        && pThisPair->m_nState != kICECandidatePairState_Succeeded )
-    {
+    if (
+        pThisPair->m_nState != kICECandidatePairState_Succeeded
+        && !FindAttributeOfType( vecAttrs.Base(), vecAttrs.Count(), k_nSTUN_Attr_UseCandidate )
+    ) {
         if ( pThisPair->m_pPeerRequest != nullptr )
         {
-            // InProgress: cancel the existing request so we don't have two in flight.
-            pThisPair->m_pPeerRequest->Cancel();
-            pThisPair->m_pPeerRequest = nullptr;
+            Assert( pThisPair->m_nState == kICECandidatePairState_InProgress );
+
+            // Retrigger the existing in-flight request rather than canceling it.
+            // Canceling would assign a new transaction ID, orphaning any response
+            // already in flight for the old ID.
+            pThisPair->m_pPeerRequest->RetriggerNow();
+        }
+        else
+        {
+            Assert( pThisPair->m_nState != kICECandidatePairState_InProgress );
+            pThisPair->m_nState = kICECandidatePairState_Waiting;
+            if ( !has_element( m_vecTriggeredCheckQueue, pThisPair ) )
+                m_vecTriggeredCheckQueue.push_back( pThisPair );
         }
-        pThisPair->m_nState = kICECandidatePairState_Waiting;
-        if ( !has_element( m_vecTriggeredCheckQueue, pThisPair ) )
-            m_vecTriggeredCheckQueue.push_back( pThisPair );
     }
 
     if ( FindAttributeOfType( vecAttrs.Base(), vecAttrs.Count(), k_nSTUN_Attr_UseCandidate ) )
     {
-        if ( pThisPair->m_nState == kICECandidatePairState_Succeeded
-                && ( m_pSelectedCandidatePair == nullptr || m_pSelectedCandidatePair == pThisPair ) )
-        {
+        if (
+            pThisPair->m_nState == kICECandidatePairState_Succeeded
+            && ( m_pSelectedCandidatePair == nullptr || m_pSelectedCandidatePair == pThisPair )
+        ) {
             SetSelectedCandidatePair( pThisPair );
         }
         else if ( m_pSelectedCandidatePair == nullptr )
         {
-            bool bAlreadyHaveANomination = ( m_pSelectedCandidatePair != nullptr );
+            bool bAlreadyHaveANomination = false;
             for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
             {
-                if ( pOtherPair->m_bNominated == true
-                    && ( pOtherPair->m_nState == kICECandidatePairState_InProgress || pOtherPair->m_nState == kICECandidatePairState_Waiting ) )
+                if (
+                    pOtherPair->m_bNominated
+                    && ( pOtherPair->m_nState == kICECandidatePairState_InProgress || pOtherPair->m_nState == kICECandidatePairState_Waiting )
+                ) {
                     bAlreadyHaveANomination = true;
-            }
-
-            // Do we already have a valid triggered check in flight?
-            if ( pThisPair->m_pPeerRequest != nullptr )
-            {
-                pThisPair->m_pPeerRequest->Cancel();
-                pThisPair->m_pPeerRequest = nullptr;
-                pThisPair->m_nState = kICECandidatePairState_Waiting;
+                }
             }
 
             if ( !bAlreadyHaveANomination )
             {
-                pThisPair->m_nState = kICECandidatePairState_Waiting;
                 pThisPair->m_bNominated = true;
-                m_vecTriggeredCheckQueue.push_back( pThisPair );
+                if ( pThisPair->m_pPeerRequest != nullptr )
+                {
+                    Assert( pThisPair->m_nState == kICECandidatePairState_InProgress );
+
+                    // The in-flight request will call SetSelectedCandidatePair when it
+                    // succeeds; m_bNominated is now set so the callback will handle it.
+                    // Retrigger rather than cancel so the existing transaction ID is preserved.
+                    pThisPair->m_pPeerRequest->RetriggerNow();
+                }
+                else
+                {
+                    Assert( pThisPair->m_nState != kICECandidatePairState_InProgress );
+                    pThisPair->m_nState = kICECandidatePairState_Waiting;
+                    m_vecTriggeredCheckQueue.push_back( pThisPair );
+                }
             }
         }
     }

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h

@@ -222,6 +222,11 @@ namespace SteamNetworkingSocketsLib {
         void Queue( uint32 nMessageType, int nEncoding, SteamNetworkingIPAddr remoteAddr, RecvSTUNPacketCallback_t cb, STUNAttribute *pExtraAttrs = nullptr, int nExtraAttrs = 0 );
         void Cancel();
 
+        // Immediately retransmit and reset the exponential backoff schedule, as if
+        // the request were freshly queued.  The transaction ID is preserved, so any
+        // response already in flight will still be matched and processed.
+        void RetriggerNow();
+
         // Handle an incoming STUN reply that has already been matched to this request
         // by transaction ID.  Verifies message integrity, fires the callback, then
         // deletes this request.