Skip to content

Add m_pPendingTurnRequest and fix flaky tests #468

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
Ibn-Butt-Toota wants to merge 1 commit into
base: master
Choose a base branch
from
+76 −53
Open

Add m_pPendingTurnRequest and fix flaky tests #468

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
88 changes: 48 additions & 40 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 @@ -1026,21 +1026,21 @@ void ICESessionInterface::QueueBindRequest( const netadr_t &addrSTUNServer, Recv

bool ICESessionInterface::QueueTURNRequest( uint32 nMsgType, int nEncoding, const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, STUNAttribute *pExtraAttrs, int nExtraAttrs )
{
Assert( !m_pPendingSTUNRequest );
m_pPendingSTUNRequest = new CSteamNetworkingSocketsSTUNRequest( this );
Assert( !m_pPendingTURNRequest );
m_pPendingTURNRequest = new CSteamNetworkingSocketsSTUNRequest( this );

if ( m_strTURNRealm.empty() )
{
m_pPendingSTUNRequest->Queue( nMsgType, nEncoding | kSTUNPacketEncodingFlags_NoMappedAddress, addrTURNServer, cb, pExtraAttrs, nExtraAttrs );
m_pPendingTURNRequest->Queue( nMsgType, nEncoding | kSTUNPacketEncodingFlags_NoMappedAddress, addrTURNServer, cb, pExtraAttrs, nExtraAttrs );
return true;
}

const int nSrvIdx = index_of( m_session.m_vecTURNServers, addrTURNServer );
if ( nSrvIdx < 0 )
{
AssertMsg( false, "TURN server address not in m_vecTURNServers" );
delete m_pPendingSTUNRequest;
m_pPendingSTUNRequest = nullptr;
delete m_pPendingTURNRequest;
m_pPendingTURNRequest = nullptr;
return false;
}
const std::string &strUsername = m_session.m_vecTURNCredentials[ nSrvIdx ].m_strUsername;
Expand All @@ -1049,8 +1049,8 @@ bool ICESessionInterface::QueueTURNRequest( uint32 nMsgType, int nEncoding, cons
if ( nExtraAttrs > 4 )
{
AssertMsg( false, "Too many extra attrs for TURN request" );
delete m_pPendingSTUNRequest;
m_pPendingSTUNRequest = nullptr;
delete m_pPendingTURNRequest;
m_pPendingTURNRequest = nullptr;
return false;
}
STUNAttribute allAttrs[ 7 ];
Expand All @@ -1073,9 +1073,9 @@ bool ICESessionInterface::QueueTURNRequest( uint32 nMsgType, int nEncoding, cons
allAttrs[ nAllAttrs ].m_pData = reinterpret_cast<const uint32*>( m_strTURNNonce.c_str() );
++nAllAttrs;

m_pPendingSTUNRequest->m_strPassword.assign( (const char*)m_arrTURNKey, sizeof( m_arrTURNKey ) );
m_pPendingTURNRequest->m_strPassword.assign( (const char*)m_arrTURNKey, sizeof( m_arrTURNKey ) );
// RFC 5766 mandates HMAC-SHA1 (not SHA256) for long-term credentials
m_pPendingSTUNRequest->Queue( nMsgType,
m_pPendingTURNRequest->Queue( nMsgType,
nEncoding | kSTUNPacketEncodingFlags_NoMappedAddress | kSTUNPacketEncodingFlags_MessageIntegrity,
addrTURNServer, cb, allAttrs, nAllAttrs );
return true;
Expand Down Expand Up @@ -1280,6 +1280,11 @@ CSteamNetworkingICESession::~CSteamNetworkingICESession()
delete pIntf->m_pPendingSTUNRequest;
pIntf->m_pPendingSTUNRequest = nullptr;
}
if ( pIntf->m_pPendingTURNRequest )
{
delete pIntf->m_pPendingTURNRequest;
pIntf->m_pPendingTURNRequest = nullptr;
}
}

for ( ICECandidatePair *pPair: m_vecCandidatePairs )
Expand Down Expand Up @@ -1531,6 +1536,11 @@ void CSteamNetworkingICESession::GatherInterfaces()
delete intf->m_pPendingSTUNRequest;
intf->m_pPendingSTUNRequest = nullptr;
}
if ( intf->m_pPendingTURNRequest )
{
delete intf->m_pPendingTURNRequest;
intf->m_pPendingTURNRequest = nullptr;
}

erase_at( m_vecInterfaces, i );
continue;
Expand Down Expand Up @@ -1573,6 +1583,21 @@ void CSteamNetworkingICESession::GatherInterfaces()
}
}

CSteamNetworkingSocketsSTUNRequest *CSteamNetworkingICESession::FindPendingRequest( ICESessionInterface *pInterface, const STUNHeader &header )
{
for ( CSteamNetworkingSocketsSTUNRequest *p : { pInterface->m_pPendingSTUNRequest, pInterface->m_pPendingTURNRequest } )
{
if ( p && p->BMatchesTransactionID( header ) )
return p;
}
for ( CSteamNetworkingSocketsSTUNRequest *p : m_vecPendingPeerRequests )
{
if ( p->m_pInterface == pInterface && p->BMatchesTransactionID( header ) )
return p;
}
return nullptr;
}

void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, ICESessionInterface *pInterface, netadr_t *pAddrRelay )
{
SteamNetworkingGlobalLock::AssertHeldByCurrentThread( "CSteamNetworkingICESession::OnPacketReceived" );
Expand Down Expand Up @@ -1663,27 +1688,7 @@ void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, IC
// STUN responses: route to the matching in-flight request by transaction ID.
if ( header.m_nMessageType != k_nSTUN_BindingRequest )
{
// Fast path: check the interface's own server-reflexive request first (O(1)).
CSteamNetworkingSocketsSTUNRequest *pRequest = pInterface->m_pPendingSTUNRequest;
if ( pRequest == nullptr
|| pRequest->m_nTransactionID[0] != header.m_nTransactionID[0]
|| pRequest->m_nTransactionID[1] != header.m_nTransactionID[1]
|| pRequest->m_nTransactionID[2] != header.m_nTransactionID[2] )
{
pRequest = nullptr;
for ( CSteamNetworkingSocketsSTUNRequest *p : m_vecPendingPeerRequests )
{
if ( p->m_pInterface == pInterface
&& p->m_nTransactionID[0] == header.m_nTransactionID[0]
&& p->m_nTransactionID[1] == header.m_nTransactionID[1]
&& p->m_nTransactionID[2] == header.m_nTransactionID[2] )
{
pRequest = p;
break;
}
}
}
if ( pRequest )
if ( CSteamNetworkingSocketsSTUNRequest *pRequest = FindPendingRequest( pInterface, header ) )
{
pRequest->ReplyPacketReceived( info, header );
}
Expand Down Expand Up @@ -1862,12 +1867,15 @@ void CSteamNetworkingICESession::OnPacketReceived( const RecvPktInfo_t &info, IC
m_pSelectedCandidatePair == nullptr
|| pThisPair->m_nPriority > m_pSelectedCandidatePair->m_nPriority
) {
// Only defer to a pending nomination on a pair at least as good as this one.
// A better pair nominated when a worse check is in flight should still win.
bool bAlreadyHaveANomination = false;
for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
{
if (
pOtherPair->m_bNominated
&& ( pOtherPair->m_nState == kICECandidatePairState_InProgress || pOtherPair->m_nState == kICECandidatePairState_Waiting )
&& !pThisPair->BIsPreferredRouteOver( *pOtherPair )
) {
bAlreadyHaveANomination = true;
}
Expand Down Expand Up @@ -1989,7 +1997,7 @@ void CSteamNetworkingICESession::Think_DiscoverRelayCandidate()
for ( const std::unique_ptr<ICESessionInterface> &pIntf : m_vecInterfaces )
{
// Skip if relay discovery is done or a request is already in flight.
if ( pIntf->m_addrTURNServer.IsValid() || pIntf->m_pPendingSTUNRequest != nullptr )
if ( pIntf->m_addrTURNServer.IsValid() || pIntf->m_pPendingTURNRequest != nullptr )
continue;

// Find the first TURN server matching this interface's address family.
Expand All @@ -2007,7 +2015,7 @@ void CSteamNetworkingICESession::Think_DiscoverRelayCandidate()
void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvSTUNPktInfo_t &info )
{
ICESessionInterface * const pIntf = info.m_pRequest->m_pInterface;
pIntf->m_pPendingSTUNRequest = nullptr;
pIntf->m_pPendingTURNRequest = nullptr;

// If we already have a result for this interface, ignore duplicates.
if ( pIntf->m_addrTURNServer.IsValid() )
Expand Down Expand Up @@ -2106,7 +2114,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST
void CSteamNetworkingICESession::STUNRequestCallback_RefreshAllocation( const RecvSTUNPktInfo_t &info )
{
ICESessionInterface * const pIntf = info.m_pRequest->m_pInterface;
pIntf->m_pPendingSTUNRequest = nullptr;
pIntf->m_pPendingTURNRequest = nullptr;

if ( info.m_pHeader != nullptr )
{
Expand Down Expand Up @@ -2150,7 +2158,7 @@ void CSteamNetworkingICESession::Think_TURNMaintenance( SteamNetworkingMicroseco
// Must have a completed relay allocation, not a busy slot, and no in-flight request.
if ( !pIntf->m_addrTURNServer.IsValid() || pIntf->m_bRelayFailed )
continue;
if ( pIntf->m_pPendingSTUNRequest != nullptr )
if ( pIntf->m_pPendingTURNRequest != nullptr )
continue;

// Refresh takes priority over CreatePermission.
Expand Down Expand Up @@ -2209,7 +2217,7 @@ void CSteamNetworkingICESession::Think_TURNMaintenance( SteamNetworkingMicroseco
}

pRequest->m_nTURNPermissionRevision = nSessionRevision;
pIntf->m_pPendingSTUNRequest = pRequest;
pIntf->m_pPendingTURNRequest = pRequest;

// Attach long-term credentials if the TURN server required auth.
if ( !pIntf->m_strTURNRealm.empty() )
Expand All @@ -2219,7 +2227,7 @@ void CSteamNetworkingICESession::Think_TURNMaintenance( SteamNetworkingMicroseco
{
AssertMsg( false, "TURN server address not in m_vecTURNServers" );
delete pRequest;
pIntf->m_pPendingSTUNRequest = nullptr;
pIntf->m_pPendingTURNRequest = nullptr;
continue;
}
const std::string &strUsername = m_vecTURNCredentials[ nSrvIdx ].m_strUsername;
Expand Down Expand Up @@ -2260,7 +2268,7 @@ void CSteamNetworkingICESession::Think_TURNMaintenance( SteamNetworkingMicroseco
void CSteamNetworkingICESession::STUNRequestCallback_CreatePermission( const RecvSTUNPktInfo_t &info )
{
ICESessionInterface * const pIntf = info.m_pRequest->m_pInterface;
pIntf->m_pPendingSTUNRequest = nullptr;
pIntf->m_pPendingTURNRequest = nullptr;

if ( info.m_pHeader == nullptr )
{
Expand Down Expand Up @@ -2617,12 +2625,12 @@ void CSteamNetworkingICESession::STUNRequestCallback_PeerConnectivityCheck( cons
}
else
{
// Once we have a selected pair, or any nominated pair (including ones queued
// but not yet sent), don't nominate more.
// Once we have a selected pair or a nominated pair at least as good as this one
// (including any queued but not yet sent), don't nominate more.
bool bAlreadyHaveANomination = ( m_pSelectedCandidatePair != nullptr );
for ( ICECandidatePair *pOtherPair : m_vecCandidatePairs )
{
if ( pOtherPair->m_bNominated )
if ( pOtherPair->m_bNominated && !pPair->BIsPreferredRouteOver( *pOtherPair ) )
bAlreadyHaveANomination = true;
}
if ( !bAlreadyHaveANomination )
Expand Down
16 changes: 14 additions & 2 deletions src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ namespace SteamNetworkingSocketsLib {
EICECandidateType CalcICECandidateType( ICECandidateKind kind, const netadr_t& addr );

/// Represents one local network interface used for ICE candidate gathering.
/// Owns its socket and tracks at most one in-flight server-reflexive STUN request.
/// Owns its socket and tracks at most one in-flight STUN bind request and one TURN request.
struct ICESessionInterface
{
// The session that owns this interface.
Expand Down Expand Up @@ -87,6 +87,11 @@ namespace SteamNetworkingSocketsLib {
// keepalive, or null if none is active. At most one per interface.
CSteamNetworkingSocketsSTUNRequest *m_pPendingSTUNRequest = nullptr;

// In-flight TURN request (Allocate, Refresh, or CreatePermission), or null if
// none is active. At most one per interface. Separate from the bind request
// so relay allocation does not have to wait for server-reflexive discovery.
CSteamNetworkingSocketsSTUNRequest *m_pPendingTURNRequest = nullptr;

// Server-reflexive discovery results. m_addrSTUNServer is also used
// as a "discovery complete" signal: it is all-zeros until a terminal
// result (success, no-NAT, or timeout) has been recorded.
Expand Down Expand Up @@ -138,7 +143,7 @@ namespace SteamNetworkingSocketsLib {
// Create and queue any TURN request, attaching long-term auth credentials when
// we have them. pExtraAttrs/nExtraAttrs are request-specific attrs (e.g.
// REQUESTED-TRANSPORT for Allocate); auth attrs are appended after them.
// Returns false and cleans up m_pPendingSTUNRequest if credential lookup fails.
// Returns false and cleans up m_pPendingTURNRequest if credential lookup fails.
bool QueueTURNRequest( uint32 nMsgType, int nEncoding, const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, STUNAttribute *pExtraAttrs, int nExtraAttrs );

ICESessionInterface( CSteamNetworkingICESession &session, uint32 nPriority, int nPrefixLen )
Expand Down Expand Up @@ -237,6 +242,10 @@ namespace SteamNetworkingSocketsLib {
// The local interface this request was sent from. Set at construction, never null.
ICESessionInterface * const m_pInterface;
uint32 m_nTransactionID[3]; // generated at construction
bool BMatchesTransactionID( const STUNHeader &header ) const
{
return memcmp( m_nTransactionID, header.m_nTransactionID, sizeof( m_nTransactionID ) ) == 0;
}
netadr_t m_remoteAddr; // Address of the peer
netadr_t m_addrRelay;
uint32 m_nMessageType;
Expand Down Expand Up @@ -507,6 +516,9 @@ namespace SteamNetworkingSocketsLib {
void STUNRequestCallback_CreatePermission( const RecvSTUNPktInfo_t &info );
void STUNRequestCallback_PeerConnectivityCheck( const RecvSTUNPktInfo_t &info );

// The in-flight request on this interface that a STUN response belongs to, or null.
CSteamNetworkingSocketsSTUNRequest *FindPendingRequest( ICESessionInterface *pInterface, const STUNHeader &header );

void OnPacketReceived( const RecvPktInfo_t &info, ICESessionInterface *pInterface, netadr_t *pAddrRelay = nullptr );
static void StaticPacketReceived( const RecvPktInfo_t &info, ICESessionInterface *pContext );
};
Expand Down
25 changes: 14 additions & 11 deletions tests/test_p2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,19 +681,22 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[],
( _CAND_IPV6_NAT, _CAND_IPV6_NAT ) ),

# Packet loss: relay can win the initial nomination race over the direct path
# (relay responses bypass the lossy adapter; see the route-upgrade commit for the
# full analysis), but the ICE client now continues probing higher-priority pairs
# after selection and upgrades when one succeeds. We therefore expect to end up
# on 'udp' (direct srflx path).
# (relay responses bypass the lossy adapter). The ICE client keeps probing
# higher priority pairs after selection and upgrades when one succeeds. We
# therefore expect to end up on 'udp' (direct srflx path).
#
# Spurious-failure analysis (at the time of this writing):
# Each STUN round trip succeeds with P = 0.9 * 0.9 = 0.81 under 10% outbound
# loss per side. The request schedule is 5 total sends (1 initial + 4 retx).
# P(all 5 fail) = 0.19^5 ~= 0.025% (~1 in 4000 runs). Triggered checks from
# the remote side add extra attempts, so the real rate is lower still.
# The route is sampled when the client closes, so the upgrade has to land within
# the tick window. Each STUN round trip succeeds with P = 0.9 * 0.9 = 0.81, and
# the direct path needs several of these in sequence (check, nomination, and the
# controlled side's own check) after the relay path has already connected. With
# the default 40 ticks (~2s after connect) a run of lost retransmits can push the
# upgrade past the close, so this case runs 100 ticks (~5s), enough for several
# full retransmit cycles (sends at 0, 0.4, 1.0, 1.9, 3.25s). Every incoming
# check from the peer also retriggers our own request immediately, which shortens
# this further.
( 'full-cone NAT, 10% packet loss',
[ '--mock-loss', '10' ] + _nat( _SRV_INT, _SRV_GW, 'full-cone' ),
[ '--mock-loss', '10' ] + _nat( _CLI_INT, _CLI_GW, 'full-cone' ),
[ '--mock-loss', '10', '--ticks', '100' ] + _nat( _SRV_INT, _SRV_GW, 'full-cone' ),
[ '--mock-loss', '10', '--ticks', '100' ] + _nat( _CLI_INT, _CLI_GW, 'full-cone' ),
'udp', 1, _CTR_DIRECT, None ),

# Signaling impairment: verify connection succeeds despite lossy or duplicate
Expand Down