From f932e64ebcc4eed366cbf892b7c8ea293dfd5833 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Mon, 6 May 2019 09:14:14 -0700 Subject: [PATCH] Fix memory leak. Two changes to cleanup. First, to it earlier, as soon as the connection switches into a state where we know we will not need to send or receive messages anymore. (Don't wait until object destruction.) Second, we were not freeing up any queued messages in the m_messagesQueued linked list. Also fix a monir bug in the linger functionality. The connection is still "connected" for all internal purposes, it just is not visible to the API anymore. So we need our stats object to stay alive, and we expect our peer to be replying in a timely manner. Add some asserts and paranoia. --- .../steamnetworkingsockets_connections.cpp | 23 +++-- .../steamnetworkingsockets_connections.h | 1 + .../clientlib/steamnetworkingsockets_snp.cpp | 92 +++++++++++++------ .../clientlib/steamnetworkingsockets_snp.h | 22 ++++- 4 files changed, 100 insertions(+), 38 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp index 2a60fac..d608c4b 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp @@ -1482,7 +1482,7 @@ void CSteamNetworkConnectionBase::ConnectionStateChanged( ESteamNetworkingConnec if ( eNewAPIState == k_ESteamNetworkingConnectionState_None ) m_queueRecvMessages.PurgeMessages(); - // Check crypto state + // Slam some stuff when we are in various states switch ( GetState() ) { case k_ESteamNetworkingConnectionState_Dead: @@ -1497,19 +1497,28 @@ void CSteamNetworkConnectionBase::ConnectionStateChanged( ESteamNetworkingConnec // And let stats tracking system know that it shouldn't // expect to be able to get stuff acked, etc m_statsEndToEnd.SetDisconnected( true, m_usecWhenEnteredConnectionState ); + + // Go head and free up memory now + SNP_ShutdownConnection(); break; case k_ESteamNetworkingConnectionState_Linger: - // Don't bother trading stats back and forth with peer, - // the only message we will send to them is "connection has been closed" - m_statsEndToEnd.SetDisconnected( true, m_usecWhenEnteredConnectionState ); - break; - case k_ESteamNetworkingConnectionState_Connected: - case k_ESteamNetworkingConnectionState_FindingRoute: // Key exchange should be complete Assert( m_bCryptKeysValid ); + + // Link stats tracker should send and expect, acks, keepalives, etc + m_statsEndToEnd.SetDisconnected( false, m_usecWhenEnteredConnectionState ); + break; + + case k_ESteamNetworkingConnectionState_FindingRoute: + + // Key exchange should be complete. (We do that when accepting a connection.) + Assert( m_bCryptKeysValid ); + + // FIXME. Probably we should NOT set the stats tracker as "connected" yet. + //Assert( m_statsEndToEnd.IsDisconnected() ); m_statsEndToEnd.SetDisconnected( false, m_usecWhenEnteredConnectionState ); break; diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h index b0e24f8..3abf195 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h @@ -623,6 +623,7 @@ protected: // void SNP_InitializeConnection( SteamNetworkingMicroseconds usecNow ); + void SNP_ShutdownConnection(); EResult SNP_SendMessage( SteamNetworkingMicroseconds usecNow, const void *pData, int cbData, int nSendFlags ); SteamNetworkingMicroseconds SNP_ThinkSendState( SteamNetworkingMicroseconds usecNow ); SteamNetworkingMicroseconds SNP_GetNextThinkTime( SteamNetworkingMicroseconds usecNow ); diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp index a106f9c..171b324 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp @@ -142,12 +142,16 @@ inline SteamNetworkingMicroseconds GetUsecPingWithFallback( CSteamNetworkConnect return nPingMS*1000; } -void SSNPSenderState::Reset() + +void SSNPSenderState::Shutdown() { - while ( !m_unackedReliableMessages.empty() ) - { - delete m_unackedReliableMessages.pop_front(); - } + m_unackedReliableMessages.delete_all(); + m_messagesQueued.delete_all(); + m_mapInFlightPacketsByPktNum.clear(); + m_listInFlightReliableRange.clear(); + m_cbPendingUnreliable = 0; + m_cbPendingReliable = 0; + m_cbSentUnackedReliable = 0; } //----------------------------------------------------------------------------- @@ -213,7 +217,6 @@ SSNPSenderState::SSNPSenderState() SSNPReceiverState::SSNPReceiverState() { // Init packet gaps with a sentinel - m_mapPacketGaps.clear(); SSNPPacketGap &sentinel = m_mapPacketGaps[INT64_MAX]; sentinel.m_nEnd = INT64_MAX; // Fixed value sentinel.m_usecWhenOKToNack = INT64_MAX; // Fixed value, for when there is nothing left to nack @@ -225,6 +228,15 @@ SSNPReceiverState::SSNPReceiverState() m_itPendingNack = m_itPendingAck; } +//----------------------------------------------------------------------------- +void SSNPReceiverState::Shutdown() +{ + m_mapUnreliableSegments.clear(); + m_bufReliableStream.clear(); + m_mapReliableStreamGaps.clear(); + m_mapPacketGaps.clear(); +} + //----------------------------------------------------------------------------- void CSteamNetworkConnectionBase::SNP_InitializeConnection( SteamNetworkingMicroseconds usecNow ) { @@ -248,6 +260,13 @@ void CSteamNetworkConnectionBase::SNP_InitializeConnection( SteamNetworkingMicro SNP_ClampSendRate(); } +//----------------------------------------------------------------------------- +void CSteamNetworkConnectionBase::SNP_ShutdownConnection() +{ + m_senderState.Shutdown(); + m_receiverState.Shutdown(); +} + //----------------------------------------------------------------------------- EResult CSteamNetworkConnectionBase::SNP_SendMessage( SteamNetworkingMicroseconds usecNow, const void *pData, int cbData, int nSendFlags ) { @@ -3071,6 +3090,13 @@ void SSNPReceiverState::DebugCheckPackGapMap() const SteamNetworkingMicroseconds CSteamNetworkConnectionBase::SNP_TimeWhenWantToSendNextPacket() const { + // We really shouldn't be trying to do this when not connected + if ( !BStateIsConnectedForWirePurposes() ) + { + AssertMsg( false, "We shouldn't be trying to send packets when not fully connected" ); + return k_nThinkTime_Never; + } + // When does the sender want to send data? SteamNetworkingMicroseconds usecNextSend = m_senderState.TimeWhenWantToSendNextPacket(); @@ -3146,33 +3172,41 @@ void CSteamNetworkConnectionBase::SNP_PopulateQuickStats( SteamNetworkingQuickCo info.m_cbPendingUnreliable = m_senderState.m_cbPendingUnreliable; info.m_cbPendingReliable = m_senderState.m_cbPendingReliable; info.m_cbSentUnackedReliable = m_senderState.m_cbSentUnackedReliable; - - // Accumulate tokens so that we can properly predict when the next time we'll be able to send something is - SNP_TokenBucket_Accumulate( usecNow ); - - // - // Time until we can send the next packet - // If anything is already queued, then that will have to go out first. Round it down - // to the nearest packet. - // - // NOTE: This ignores the precise details of SNP framing. If there are tons of - // small packets, it'll actually be worse. We might be able to approximate that - // the framing overhead better by also counting up the number of *messages* pending. - // Probably not worth it here, but if we had that number available, we'd use it. - int cbPendingTotal = m_senderState.PendingBytesTotal() / k_cbSteamNetworkingSocketsMaxMessageNoFragment * k_cbSteamNetworkingSocketsMaxMessageNoFragment; - - // Adjust based on how many tokens we have to spend now (or if we are already - // over-budget and have to wait until we could spend another) - cbPendingTotal -= (int)m_senderState.m_flTokenBucket; - if ( cbPendingTotal <= 0 ) + if ( GetState() == k_ESteamNetworkingConnectionState_Connected ) { - // We could send it right now. - info.m_usecQueueTime = 0; + + // Accumulate tokens so that we can properly predict when the next time we'll be able to send something is + SNP_TokenBucket_Accumulate( usecNow ); + + // + // Time until we can send the next packet + // If anything is already queued, then that will have to go out first. Round it down + // to the nearest packet. + // + // NOTE: This ignores the precise details of SNP framing. If there are tons of + // small packets, it'll actually be worse. We might be able to approximate that + // the framing overhead better by also counting up the number of *messages* pending. + // Probably not worth it here, but if we had that number available, we'd use it. + int cbPendingTotal = m_senderState.PendingBytesTotal() / k_cbSteamNetworkingSocketsMaxMessageNoFragment * k_cbSteamNetworkingSocketsMaxMessageNoFragment; + + // Adjust based on how many tokens we have to spend now (or if we are already + // over-budget and have to wait until we could spend another) + cbPendingTotal -= (int)m_senderState.m_flTokenBucket; + if ( cbPendingTotal <= 0 ) + { + // We could send it right now. + info.m_usecQueueTime = 0; + } + else + { + + info.m_usecQueueTime = (int64)cbPendingTotal * k_nMillion / SNP_ClampSendRate(); + } } else { - - info.m_usecQueueTime = (int64)cbPendingTotal * k_nMillion / SNP_ClampSendRate(); + // We'll never be able to send it. (Or, we don't know when that will be.) + info.m_usecQueueTime = INT64_MAX; } } diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h index e50801d..2b5fd9c 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h @@ -119,6 +119,14 @@ struct SSNPSendMessageList return false; } + /// Delete all elements. This should only be called if you own the messages! + void delete_all() + { + while ( m_pFirst ) + delete pop_front(); + Assert( m_pLast == nullptr ); + } + /// Unlink the message at the head, if any and return it. /// Unlike STL pop_front, this will return nullptr if the /// list is empty @@ -172,9 +180,9 @@ struct SSNPSenderState { SSNPSenderState(); ~SSNPSenderState() { - Reset(); + Shutdown(); } - void Reset(); + void Shutdown(); /// Current sending rate in bytes per second, RFC 3448 4.2 states default /// is one packet per second, but that is insane and we're not doing that. @@ -352,6 +360,10 @@ struct SSNPPacketGap struct SSNPReceiverState { SSNPReceiverState(); + ~SSNPReceiverState() { + Shutdown(); + } + void Shutdown(); /// Unreliable message segments that we have received. When an unreliable message /// needs to be fragmented, we store the pieces here. NOTE: it might be more efficient @@ -444,6 +456,12 @@ struct SSNPReceiverState /// if we don't have any acks pending right now. inline SteamNetworkingMicroseconds TimeWhenFlushAcks() const { + // Paranoia + if ( m_mapPacketGaps.empty() ) + { + AssertMsg( false, "TimeWhenFlushAcks - we're shut down!" ); + return INT64_MAX; + } return m_itPendingAck->second.m_usecWhenAckPrior; }