From 490c8cebfe0a08c2edbf5e0de4999f58eaa8aaf4 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Thu, 28 May 2020 10:49:34 -0700 Subject: [PATCH] Add more paranoia checking. Paranoia checking now toggled using STEAMNETWORKINGSOCKETS_SNP_PARANOIA define instead of _DEBUG. And it is always set to the max for now. I should make that a bit more flexible, but since right now I know I have some issues, I want to leave it on. --- .../clientlib/steamnetworkingsockets_snp.cpp | 57 ++++++++++++++++--- .../clientlib/steamnetworkingsockets_snp.h | 26 ++++++++- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp index 1f16e2b..ad1ead1 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp @@ -182,8 +182,38 @@ SSNPSenderState::SSNPSenderState() sentinel.m_pTransport = nullptr; sentinel.m_usecWhenSent = 0; m_itNextInFlightPacketToTimeout = m_mapInFlightPacketsByPktNum.end(); + DebugCheckInFlightPacketMap(); } +#if STEAMNETWORKINGSOCKETS_SNP_PARANOIA > 0 +void SSNPSenderState::DebugCheckInFlightPacketMap() const +{ + Assert( !m_mapInFlightPacketsByPktNum.empty() ); + bool bFoundNextToTimeout = false; + auto it = m_mapInFlightPacketsByPktNum.begin(); + Assert( it->first == INT64_MIN ); + Assert( m_itNextInFlightPacketToTimeout != it ); + int64 prevPktNum = it->first; + SteamNetworkingMicroseconds prevWhenSent = it->second.m_usecWhenSent; + while ( ++it != m_mapInFlightPacketsByPktNum.end() ) + { + Assert( prevPktNum < it->first ); + Assert( prevWhenSent <= it->second.m_usecWhenSent ); + if ( it == m_itNextInFlightPacketToTimeout ) + { + Assert( !bFoundNextToTimeout ); + bFoundNextToTimeout = true; + } + prevPktNum = it->first; + prevWhenSent = it->second.m_usecWhenSent; + } + if ( !bFoundNextToTimeout ) + { + Assert( m_itNextInFlightPacketToTimeout == m_mapInFlightPacketsByPktNum.end() ); + } +} +#endif + //----------------------------------------------------------------------------- SSNPReceiverState::SSNPReceiverState() { @@ -515,10 +545,10 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa // Make sure we have initialized the connection Assert( BStateIsActive() ); - SteamNetworkingMicroseconds usecNow = ctx.m_usecNow; - int64 nPktNum = ctx.m_nPktNum; + const SteamNetworkingMicroseconds usecNow = ctx.m_usecNow; + const int64 nPktNum = ctx.m_nPktNum; - int nLogLevelPacketDecode = m_connectionConfig.m_LogLevel_PacketDecode.Get(); + const int nLogLevelPacketDecode = m_connectionConfig.m_LogLevel_PacketDecode.Get(); SpewVerboseGroup( nLogLevelPacketDecode, "[%s] decode pkt %lld\n", GetDescription(), (long long)nPktNum ); // Decode frames until we get to the end of the payload @@ -766,6 +796,16 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa // Ack // + #if STEAMNETWORKINGSOCKETS_SNP_PARANOIA > 0 + m_senderState.DebugCheckInFlightPacketMap(); + #if STEAMNETWORKINGSOCKETS_SNP_PARANOIA == 1 + if ( ( nPktNum & 255 ) == 0 ) // only do it periodically + #endif + { + m_senderState.DebugCheckInFlightPacketMap(); + } + #endif + // Parse latest received sequence number int64 nLatestRecvSeqNum; { @@ -994,7 +1034,7 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa // No need to track this anymore, remove from our table inFlightPkt = m_senderState.m_mapInFlightPacketsByPktNum.erase( inFlightPkt ); --inFlightPkt; - Assert( !m_senderState.m_mapInFlightPacketsByPktNum.empty() ); + m_senderState.MaybeCheckInFlightPacketMap(); } // Ack of in-flight end-to-end stats? @@ -1121,8 +1161,8 @@ void CSteamNetworkConnectionBase::SNP_SenderProcessPacketNack( int64 nPktNum, SN SteamNetworkingMicroseconds CSteamNetworkConnectionBase::SNP_SenderCheckInFlightPackets( SteamNetworkingMicroseconds usecNow ) { // Fast path for nothing in flight. - Assert( !m_senderState.m_mapInFlightPacketsByPktNum.empty() ); - if ( m_senderState.m_mapInFlightPacketsByPktNum.size() == 1 ) + m_senderState.MaybeCheckInFlightPacketMap(); + if ( m_senderState.m_mapInFlightPacketsByPktNum.size() <= 1 ) { Assert( m_senderState.m_itNextInFlightPacketToTimeout == m_senderState.m_mapInFlightPacketsByPktNum.end() ); return k_nThinkTime_Never; @@ -1186,6 +1226,9 @@ SteamNetworkingMicroseconds CSteamNetworkConnectionBase::SNP_SenderCheckInFlight break; } + // Make sure we didn't hose data structures + m_senderState.MaybeCheckInFlightPacketMap(); + // Return time when we really need to check back in again. // We don't wake up early just to expire old nacked packets, // there is no urgency or value in doing that, we can clean @@ -3169,7 +3212,7 @@ void SSNPReceiverState::QueueFlushAllAcks( SteamNetworkingMicroseconds usecWhen } } -#ifdef _DEBUG +#if STEAMNETWORKINGSOCKETS_SNP_PARANOIA > 1 void SSNPReceiverState::DebugCheckPackGapMap() const { int64 nPrevEnd = 0; diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h index 6b753f1..77a2359 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h @@ -7,6 +7,14 @@ #include #include +// Paranoia level: +// 0 = disabled +// 1 = sometimes +// 2 = max + +// !KLUDGE! Always enable this until we track down this memory thing +#define STEAMNETWORKINGSOCKETS_SNP_PARANOIA 2 + struct P2PSessionState_t; namespace SteamNetworkingSocketsLib { @@ -356,6 +364,18 @@ struct SSNPSenderState // Remove messages from m_unackedReliableMessages that have been fully acked. void RemoveAckedReliableMessageFromUnackedList(); + + /// Check invariants in debug. + #if STEAMNETWORKINGSOCKETS_SNP_PARANOIA == 0 + inline void DebugCheckInFlightPacketMap() const {} + #else + void DebugCheckInFlightPacketMap() const; + #endif + #if STEAMNETWORKINGSOCKETS_SNP_PARANOIA > 1 + inline void MaybeCheckInFlightPacketMap() const { DebugCheckInFlightPacketMap(); } + #else + inline void MaybeCheckInFlightPacketMap() const {} + #endif }; struct SSNPRecvUnreliableSegmentKey @@ -448,7 +468,7 @@ struct SSNPReceiverState /// The next ack that needs to be sent. The invariant /// for the times are: /// - /// * Blocks with lower pakcet numbers: m_usecWhenAckPrior = INT64_MAX + /// * Blocks with lower packet numbers: m_usecWhenAckPrior = INT64_MAX /// * This block: m_usecWhenAckPrior < INT64_MAX, or we are the sentinel /// * Blocks with higher packet numbers (if we are not the sentinel): m_usecWhenAckPrior >= previous m_usecWhenAckPrior /// @@ -459,7 +479,7 @@ struct SSNPReceiverState /// many as will fit. The one exception is that if /// sending an ack would imply a NACK that we don't want to /// send yet. (Remember the restrictions on what we are able - /// to commununicate due to the tight RLE encoding of the wire + /// to communicate due to the tight RLE encoding of the wire /// format.) These delays are usually very short lived, and /// only happen when there is packet loss, so they don't delay /// acks very much. The whole purpose of this rather involved @@ -495,7 +515,7 @@ struct SSNPReceiverState } /// Check invariants in debug. - #ifdef _DEBUG + #if STEAMNETWORKINGSOCKETS_SNP_PARANOIA > 1 void DebugCheckPackGapMap() const; #else inline void DebugCheckPackGapMap() const {}