Skip to content

Commit 8bd39a3

Browse files
committed
SNP stop waiting improvements
- Tweak the clamp of the stop_waiting value to better align with the actual invariant. We clamp to the packet just below the sentinel packet gap in the map. - Also add a few map spotchecks in places where they are basically free, perf-wise. - And if somehow we still do get past the end of the map, just assert and break the loop, don't kill the connection. This is in response to issue #414. I haven't been able to reproduce it, and I still don't have a theory for how it is possible, outside of a memory scribble. Still, I believe that this change is good. P4:10699208 (cherry picked from commit 61f7586)
1 parent 1497e2d commit 8bd39a3

1 file changed

Lines changed: 17 additions & 9 deletions

File tree

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,9 +1014,11 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
10141014
// Clamping the requested stop waiting point to the highest packet number we
10151015
// received keeps a lot of code simple. If this clamp activates, the gap map
10161016
// will get totally emptied by the loop below.
1017-
if ( unlikely( nMinPktNumToSendAcks > m_statsEndToEnd.m_nMaxRecvPktNum ) )
1017+
const int nPktNumBeforeSentinelGap = m_receiverState.m_mapPacketGaps.rbegin()->first-1;
1018+
Assert( nPktNumBeforeSentinelGap <= m_statsEndToEnd.m_nMaxRecvPktNum );
1019+
if ( unlikely( nMinPktNumToSendAcks > nPktNumBeforeSentinelGap ) )
10181020
{
1019-
nMinPktNumToSendAcks = m_statsEndToEnd.m_nMaxRecvPktNum;
1021+
nMinPktNumToSendAcks = nPktNumBeforeSentinelGap;
10201022
}
10211023

10221024
if ( nMinPktNumToSendAcks == m_receiverState.m_nMinPktNumToSendAcks )
@@ -1047,16 +1049,17 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
10471049
auto h = m_receiverState.m_mapPacketGaps.begin();
10481050
while ( h->first <= m_receiverState.m_nMinPktNumToSendAcks )
10491051
{
1052+
Assert( h->first < h->second.m_nEnd );
10501053
if ( h->second.m_nEnd > m_receiverState.m_nMinPktNumToSendAcks )
10511054
{
10521055

1053-
// We should never reach the sentinel, due to the clamp
1054-
// against m_statsEndToEnd.m_nMaxRecvPktNum above. But we will
1055-
// add a little paranoia check here, just in case.
1056-
if ( unlikely( h->second.m_nEnd == INT64_MAX ) )
1056+
// We should never reach the sentinel, due to the clamp above.
1057+
// But we will add a little paranoia check here, just in case.
1058+
if ( unlikely( h->first > nPktNumBeforeSentinelGap ) )
10571059
{
1058-
AssertMsgOnce( false, "SNP stop waiting advanced past sentinel gap. This should never happen!" );
1059-
DECODE_ERROR( "stop_waiting past sentinel gap" );
1060+
Assert( h == m_receiverState.m_mapPacketGaps.end() );
1061+
AssertMsg( false, "SNP stop waiting advanced past sentinel gap. This should never happen!" );
1062+
break;
10601063
}
10611064

10621065
// Ug. You're not supposed to modify the key in a map.
@@ -1080,8 +1083,13 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
10801083
++m_receiverState.m_itPendingNack;
10811084
}
10821085

1083-
// Packet loss is in the past. Forget about it and move on
1086+
// Packet loss is in the past. Forget about it and move on. While we're here,
1087+
// let's spot check that the map is not hosed.
1088+
#ifdef DBGFLAG_ASSERT
1089+
int64 nCheckPrev = h->second.m_nEnd;
1090+
#endif
10841091
h = m_receiverState.m_mapPacketGaps.erase(h);
1092+
AssertMsg( h->first > nCheckPrev, "PacketGaps map not increasing" );
10851093
}
10861094

10871095
SNP_DebugCheckPacketGapMap();

0 commit comments

Comments
 (0)