Skip to content

Commit 4965650

Browse files
committed
ICE client: always reply to binding requests; upgrade route if better pair succeeds
Previously, binding requests from non-selected pairs were silently discarded once a candidate pair was nominated. There is no good reason to ignore them -- the response is cheap and the RFC expects it -- so remove that guard entirely. Also allow the controlling side to upgrade to a higher-priority pair after initial nomination. Under packet loss the relay path can win the nomination race over a direct path (relay responses bypass the lossy adapter), leaving the connection on a suboptimal route. Now, if a higher-priority pair succeeds after selection, it is nominated and SetSelectedCandidatePair is called again to switch to it. On the controlled side, the USE-CANDIDATE and nomination guards are relaxed in the same way, so they honor an upgrade request from the controlling peer. The response callback now always transitions pair state (Succeeded or Failed) before the priority guard. Previously the guard returned early after clearing m_pPeerRequest but before updating m_nState, leaving pairs stuck in InProgress with no active request and breaking the invariant assumed by the binding request handler.
1 parent 4bfd2cb commit 4965650

1 file changed

Lines changed: 36 additions & 24 deletions

File tree

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1705,12 +1705,6 @@ void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, IC
17051705
}
17061706
}
17071707

1708-
// Stale request on a pair we're not using? Ignore.
1709-
if ( m_pSelectedCandidatePair != nullptr && m_pSelectedCandidatePair != pThisPair )
1710-
{
1711-
return;
1712-
}
1713-
17141708
//
17151709
// Didn't find a pair? Then maybe we can create one
17161710
//
@@ -1770,12 +1764,16 @@ void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, IC
17701764
{
17711765
if (
17721766
pThisPair->m_nState == kICECandidatePairState_Succeeded
1773-
&& ( m_pSelectedCandidatePair == nullptr || m_pSelectedCandidatePair == pThisPair )
1767+
&& ( m_pSelectedCandidatePair == nullptr
1768+
|| m_pSelectedCandidatePair == pThisPair
1769+
|| pThisPair->m_nPriority > m_pSelectedCandidatePair->m_nPriority )
17741770
) {
17751771
SetSelectedCandidatePair( pThisPair );
17761772
}
1777-
else if ( m_pSelectedCandidatePair == nullptr )
1778-
{
1773+
else if (
1774+
m_pSelectedCandidatePair == nullptr
1775+
|| pThisPair->m_nPriority > m_pSelectedCandidatePair->m_nPriority
1776+
) {
17791777
bool bAlreadyHaveANomination = false;
17801778
for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
17811779
{
@@ -2334,38 +2332,52 @@ void CSteamNetworkingICESession::STUNRequestCallback_PeerConnectivityCheck( cons
23342332
const SteamNetworkingMicroseconds usPing = Max( SteamNetworkingMicroseconds( 1 ), info.m_usecNow - info.m_pRequest->m_usecLastSentTime );
23352333
pPair->m_nLastRecordedPing = Max( 1, (int)( usPing / 1000 ) );
23362334

2337-
// Stale request on a pair we're not using? Ignore.
2338-
if ( m_pSelectedCandidatePair != nullptr && m_pSelectedCandidatePair != pPair )
2335+
// Always update the pair state so it is never left stuck in InProgress with no
2336+
// active request (which would break the invariant checked in OnPacketReceived).
2337+
if ( info.m_pHeader == nullptr )
23392338
{
2339+
pPair->m_nState = kICECandidatePairState_Failed;
23402340
return;
23412341
}
2342+
pPair->m_nState = kICECandidatePairState_Succeeded;
23422343

2343-
if ( info.m_pHeader == nullptr )
2344+
// Don't nominate or upgrade to a pair with lower-or-equal priority than the
2345+
// current selection — it can't improve our route.
2346+
if ( m_pSelectedCandidatePair != nullptr && m_pSelectedCandidatePair != pPair
2347+
&& pPair->m_nPriority <= m_pSelectedCandidatePair->m_nPriority )
23442348
{
2345-
pPair->m_nState = kICECandidatePairState_Failed;
23462349
return;
23472350
}
2348-
pPair->m_nState = kICECandidatePairState_Succeeded;
2351+
23492352
if ( pPair->m_bNominated )
23502353
{
23512354
SetSelectedCandidatePair( pPair );
23522355
}
23532356
else if ( m_role == k_EICERole_Controlling )
23542357
{
2355-
// Once we have a selected pair, or any nominated pair (including ones queued
2356-
// but not yet sent), don't nominate more.
2357-
bool bAlreadyHaveANomination = ( m_pSelectedCandidatePair != nullptr );
2358-
for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
2359-
{
2360-
if ( pOtherPair->m_bNominated )
2361-
bAlreadyHaveANomination = true;
2362-
}
2363-
if ( !bAlreadyHaveANomination )
2358+
if ( m_pSelectedCandidatePair != nullptr
2359+
&& pPair->m_nPriority > m_pSelectedCandidatePair->m_nPriority )
23642360
{
2361+
// Better path than current selection — nominate it to trigger an upgrade.
23652362
pPair->m_bNominated = true;
23662363
m_vecTriggeredCheckQueue.push_back( pPair );
23672364
}
2368-
2365+
else
2366+
{
2367+
// Once we have a selected pair, or any nominated pair (including ones queued
2368+
// but not yet sent), don't nominate more.
2369+
bool bAlreadyHaveANomination = ( m_pSelectedCandidatePair != nullptr );
2370+
for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
2371+
{
2372+
if ( pOtherPair->m_bNominated )
2373+
bAlreadyHaveANomination = true;
2374+
}
2375+
if ( !bAlreadyHaveANomination )
2376+
{
2377+
pPair->m_bNominated = true;
2378+
m_vecTriggeredCheckQueue.push_back( pPair );
2379+
}
2380+
}
23692381
}
23702382
}
23712383

0 commit comments

Comments
 (0)