Skip to content

ICE: Dedupe servers and track the server index on STUN and TURN requests #438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kwy404 wants to merge 1 commit into
base: master
Choose a base branch
from
+57 −20
Open

ICE: Dedupe servers and track the server index on STUN and TURN requests #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ bool ICESessionInterface::QueueTURNRequest( uint32 nMsgType, int nEncoding, cons
return true;
}

void ICESessionInterface::QueueAllocateRequest( const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, int nEncoding )
void ICESessionInterface::QueueAllocateRequest( int nTURNServerIdx, RecvSTUNPacketCallback_t cb, int nEncoding )
{
// REQUESTED-TRANSPORT: UDP (IANA protocol 17), protocol byte + 3 RFFU bytes
uint32 uTransport = htonl( 17u << 24 );
Expand All @@ -1090,7 +1090,8 @@ void ICESessionInterface::QueueAllocateRequest( const netadr_t &addrTURNServer,
reqTransport.m_nLength = 4;
reqTransport.m_pData = &uTransport;

QueueTURNRequest( k_nTURN_AllocateRequest, nEncoding, addrTURNServer, cb, &reqTransport, 1 );
if ( QueueTURNRequest( k_nTURN_AllocateRequest, nEncoding, m_session.m_vecTURNServers[ nTURNServerIdx ], cb, &reqTransport, 1 ) )
m_pPendingSTUNRequest->m_nServerIdx = nTURNServerIdx;
}

void ICESessionInterface::QueueRefreshRequest( RecvSTUNPacketCallback_t cb, int nEncoding )
Expand Down Expand Up @@ -1211,7 +1212,10 @@ CSteamNetworkingICESession::CSteamNetworkingICESession( const ICESessionConfig&
{
netadr_t adr;
SteamNetworkingIPAddrToNetAdr( adr, ip );
m_vecSTUNServers.push_back( adr );

// Skip duplicates, so a dead server listed twice is only tried once.
if ( index_of( m_vecSTUNServers, adr ) < 0 )
m_vecSTUNServers.push_back( adr );
}
}
}
Expand All @@ -1235,6 +1239,11 @@ CSteamNetworkingICESession::CSteamNetworkingICESession( const ICESessionConfig&
{
netadr_t adr;
SteamNetworkingIPAddrToNetAdr( adr, ip );

// Skip duplicates, keeping the first entry's credentials. Credentials
// are looked up by address, so a later duplicate could never use its own.
if ( index_of( m_vecTURNServers, adr ) >= 0 )
continue;
m_vecTURNServers.push_back( adr );
TURNCredentials cred;
cred.m_strUsername = pszUsername;
Expand Down Expand Up @@ -1957,12 +1966,13 @@ void CSteamNetworkingICESession::Think_DiscoverServerReflexiveCandidates()
continue;

// Find the first STUN server matching this interface's address family.
for ( const netadr_t &srv : m_vecSTUNServers )
for ( int idx = 0; idx < len( m_vecSTUNServers ); ++idx )
{
if ( srv.GetType() == pIntf->m_boundAddr.GetType() )
if ( m_vecSTUNServers[idx].GetType() == pIntf->m_boundAddr.GetType() )
{
++TEST_ICE_ctr_srflx_send;
pIntf->QueueBindRequest( srv, &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding | kSTUNPacketEncodingFlags_MappedAddress );
pIntf->QueueBindRequest( m_vecSTUNServers[idx], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding | kSTUNPacketEncodingFlags_MappedAddress );
pIntf->m_pPendingSTUNRequest->m_nServerIdx = idx;
break;
}
}
Expand All @@ -1985,11 +1995,11 @@ void CSteamNetworkingICESession::Think_DiscoverRelayCandidate()
continue;

// Find the first TURN server matching this interface's address family.
for ( const netadr_t &srv : m_vecTURNServers )
for ( int idx = 0; idx < len( m_vecTURNServers ); ++idx )
{
if ( srv.GetType() == pIntf->m_boundAddr.GetType() )
if ( m_vecTURNServers[idx].GetType() == pIntf->m_boundAddr.GetType() )
{
pIntf->QueueAllocateRequest( srv, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
pIntf->QueueAllocateRequest( idx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
break;
}
}
Expand Down Expand Up @@ -2067,7 +2077,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST
CUtlNetAdrRender( info.m_pRequest->m_remoteAddr ).String(), cred.m_strUsername.c_str() );

// Re-queue the allocate with credentials.
pIntf->QueueAllocateRequest( info.m_pRequest->m_remoteAddr, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
pIntf->QueueAllocateRequest( info.m_pRequest->m_nServerIdx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
return;
}
}
Expand All @@ -2081,9 +2091,8 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST
}

// Timed out -- try the next TURN server if available.
const int nTURNServerIdx = index_of( m_vecTURNServers, info.m_pRequest->m_remoteAddr );
const int nNextTURNServerIdx = nTURNServerIdx + 1;
if ( nTURNServerIdx < 0 || nNextTURNServerIdx >= len( m_vecTURNServers ) )
const int nNextTURNServerIdx = info.m_pRequest->m_nServerIdx + 1;
if ( nNextTURNServerIdx >= len( m_vecTURNServers ) )
{
// Exhausted all TURN servers. Mark failed.
pIntf->m_addrTURNServer = info.m_pRequest->m_remoteAddr;
Expand All @@ -2092,7 +2101,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST
}

// Try the next TURN server.
pIntf->QueueAllocateRequest( m_vecTURNServers[nNextTURNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
pIntf->QueueAllocateRequest( nNextTURNServerIdx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );
}

void CSteamNetworkingICESession::STUNRequestCallback_RefreshAllocation( const RecvSTUNPktInfo_t &info )
Expand Down Expand Up @@ -2294,9 +2303,8 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate( c
}

// Timed out to this STUN server -- try the next one if available.
const int nSTUNServerIdx = index_of( m_vecSTUNServers, info.m_pRequest->m_remoteAddr );
const int nNextSTUNServerIdx = nSTUNServerIdx + 1;
if ( nSTUNServerIdx < 0 || nNextSTUNServerIdx >= len( m_vecSTUNServers ) )
const int nNextSTUNServerIdx = info.m_pRequest->m_nServerIdx + 1;
if ( nNextSTUNServerIdx >= len( m_vecSTUNServers ) )
{
// Exhausted all STUN servers. Mark failed so Think_DiscoverServerReflexiveCandidates
// does not retry this interface indefinitely.
Expand All @@ -2307,6 +2315,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate( c

// Try the next server
pIntf->QueueBindRequest( m_vecSTUNServers[nNextSTUNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding );
pIntf->m_pPendingSTUNRequest->m_nServerIdx = nNextSTUNServerIdx;
}

void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive( const RecvSTUNPktInfo_t &info )
Expand All @@ -2329,9 +2338,9 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive( c
if ( m_vecSTUNServers.empty() )
return;

const int nSTUNServerIdx = std::max( 0, index_of( m_vecSTUNServers, info.m_pRequest->m_remoteAddr ) );
const int nNextSTUNServerIdx = ( nSTUNServerIdx + 1 ) % len( m_vecSTUNServers );
const int nNextSTUNServerIdx = ( info.m_pRequest->m_nServerIdx + 1 ) % len( m_vecSTUNServers );
pIntf->QueueBindRequest( m_vecSTUNServers[ nNextSTUNServerIdx ], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive, m_nEncoding );
pIntf->m_pPendingSTUNRequest->m_nServerIdx = nNextSTUNServerIdx;
}

void CSteamNetworkingICESession::UpdateKeepalive( ICESessionInterface *pIntf )
Expand All @@ -2342,6 +2351,7 @@ void CSteamNetworkingICESession::UpdateKeepalive( ICESessionInterface *pIntf )
return;

pIntf->QueueBindRequest( pIntf->m_addrSTUNServer, &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive, m_nEncoding );
pIntf->m_pPendingSTUNRequest->m_nServerIdx = std::max( 0, index_of( m_vecSTUNServers, pIntf->m_addrSTUNServer ) );
}

void CSteamNetworkingICESession::Think_KeepAliveOnCandidates( SteamNetworkingMicroseconds usecNow )
Expand Down
6 changes: 5 additions & 1 deletion src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ namespace SteamNetworkingSocketsLib {
void QueueBindRequest( const netadr_t &addrSTUNServer, RecvSTUNPacketCallback_t cb, int nEncoding );

// Send a TURN Allocate request
void QueueAllocateRequest( const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, int nEncoding );
void QueueAllocateRequest( int nTURNServerIdx, RecvSTUNPacketCallback_t cb, int nEncoding );

// Send a TURN Refresh request to keep the allocation alive
void QueueRefreshRequest( RecvSTUNPacketCallback_t cb, int nEncoding );
Expand Down Expand Up @@ -250,6 +250,10 @@ namespace SteamNetworkingSocketsLib {
// Not used for other request types
int m_nTURNPermissionRevision;

// For STUN binding and TURN Allocate requests, the index of the server in the
// session's STUN or TURN server list. Used to fail over to the next entry.
int m_nServerIdx = 0;

// Serialize the packet and start the retry loop.
void Queue( uint32 nMessageType, int nEncoding, netadr_t remoteAddr, RecvSTUNPacketCallback_t cb, STUNAttribute *pExtraAttrs = nullptr, int nExtraAttrs = 0 );

Expand Down
23 changes: 23 additions & 0 deletions tests/test_p2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,17 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[],
},
expected_candidates=( {'host': 1}, {'host': 1} ) ) ),

# Same as above, but the dead STUN server is listed twice (like several hostnames
# resolving to one IP). The duplicate is skipped, so the server is tried once and
# then discovery gives up, instead of failing over to the same entry forever. The
# longer connection timeout leaves room for more attempts, which must not happen.
( 'STUN unavailable, duplicate server entries',
_nat( _SRV_INT, _SRV_GW, 'full-cone' ) + [ '--timeout-ms', '16000' ],
_nat( _CLI_INT, _CLI_GW, 'full-cone' ) + [ '--timeout-ms', '16000' ],
dict( stun='%s,%s' % ( _DEAD_SERVER, _DEAD_SERVER ), turn=None,
expected_counters={ 'binding_req_retx': (4, 4) },
expected_candidates=( {'host': 1}, {'host': 1} ) ) ),

# TURN not configured: symmetric NAT requires relay; without it the connection must fail.
# Connectivity checks to srflx candidates retransmit 4 times before giving up.
( 'TURN not configured (symmetric NAT)',
Expand Down Expand Up @@ -475,6 +486,18 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[],
},
expected_candidates=( _CAND_NAT_NO_TURN, _CAND_NAT_NO_TURN ) ) ),

# Same as above, but the dead TURN server is listed twice. The duplicate is
# skipped, so there is one allocate attempt and then relay discovery gives up.
( 'TURN unreachable, duplicate server entries (symmetric NAT)',
_nat( _SRV_INT, _SRV_GW, 'symmetric' ) + [ '--timeout-ms', '16000' ],
_nat( _CLI_INT, _CLI_GW, 'symmetric' ) + [ '--timeout-ms', '16000' ],
dict( turn='%s,%s' % ( _DEAD_SERVER, _DEAD_SERVER ),
expected_counters={
'allocate_send': (1, 1),
'allocate_retx': (4, 4),
},
expected_candidates=( _CAND_NAT_NO_TURN, _CAND_NAT_NO_TURN ) ) ),

# TURN wrong password: the server sends a 401 challenge; the client retries with a
# bad HMAC (wrong password) and gets a second 401, marking relay as failed.
# allocate_send=2: initial (no auth) + one retry (wrong credentials).
Expand Down