From 9f81579f207d68b0fdc9216c756bd3d4c8eccbd6 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Mon, 1 Nov 2021 18:10:18 -0700 Subject: [PATCH] Fix issues with refcounting of reliable segments. If our ping is very high and send rate is low, we can easily receive ACKs for all outstanding reliable data of a message we have sent, while it is still the first item in the queue and we still have more to send for that message. If this happens, we would decrement the last reference count and delete the msssage, even though we haven't finished sending it. To fix this, just have the queue hold a reference to the message. P4:6868998 --- .../clientlib/steamnetworkingsockets_snp.cpp | 16 +++++++++++++++- .../clientlib/steamnetworkingsockets_snp.h | 12 ++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp index 1bc60b9..58d66c8 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp @@ -216,6 +216,10 @@ void SSNPSenderState::RemoveRefCountReliableSegment( uint16 hSeg ) if ( --info.m_nSentReliableSegRefCount <= 0 ) { DbgAssert( info.m_nSentReliableSegRefCount == 0 ); + + // We should not be in the lane send queue + Assert( pMsg->m_linksSecondaryQueue.m_pQueue == nullptr ); + pMsg->Unlink(); pMsg->Release(); } @@ -357,7 +361,7 @@ int64 CSteamNetworkConnectionBase::SNP_SendMessage( CSteamNetworkingMessage *pSe hdrEnd = SerializeVarInt( hdrEnd, cbData>>5U ); } reliableInfo.m_cbHdr = hdrEnd - hdr; - reliableInfo.m_nSentReliableSegRefCount = 0; + reliableInfo.m_nSentReliableSegRefCount = 1; // Initialize reference count to 1. // Grow the total size of the message by the header pSendMessage->m_cbSize += reliableInfo.m_cbHdr; @@ -2514,6 +2518,7 @@ int CSteamNetworkConnectionBase::SNP_SerializePacketInternal( SNPPacketSerialize // sure that we don't make an excessively large // one and then have a hard time retrying it later. int cbDesiredSegSize = pSendMsg->m_cbSize - sendLane.m_cbCurrentSendMessageSent; + Assert( cbDesiredSegSize > 0 ); if ( cbDesiredSegSize > m_cbMaxReliableMessageSegment ) { cbDesiredSegSize = m_cbMaxReliableMessageSegment; @@ -2635,6 +2640,15 @@ int CSteamNetworkConnectionBase::SNP_SerializePacketInternal( SNPPacketSerialize if ( !k_bUnreliableOnly && pSendMsg->SNPSend_IsReliable() ) { + // We hold a reference while in the lane send queue. But + // we've been removed, so decrement that reference count now. + // Note that this might drop our reference count to zero. + // But there should be at least one segment ready to be serialized + // which will hold a reference to us. We just haven't added it yet. + CSteamNetworkingMessage::ReliableSendInfo_t &relInfo = pSendMsg->ReliableSendInfo(); + Assert( relInfo.m_nSentReliableSegRefCount > 0 ); + --relInfo.m_nSentReliableSegRefCount; + // Go ahead and add us to the end of the list of unacked messages pSeg->m_pMsg->LinkToQueueTail( &CSteamNetworkingMessage::m_links, &m_senderState.m_unackedReliableMessages ); } diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h index 939cc8d..5b00f1e 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.h @@ -128,10 +128,12 @@ public: inline void SNPSend_SetReliableStreamPos( int64 x ) { Assert( m_nFlags & k_nSteamNetworkingSend_Reliable ); ReliableSendInfo().m_nStreamPos = x; } // Working data for reliable messages. - // !KLUDGE! Reuse the identity field struct ReliableSendInfo_t { int64 m_nStreamPos; + + // Number of reliable segments that refer to this message. + // Also while we are in the queue waiting to be sent the queue holds a reference int m_nSentReliableSegRefCount; int m_cbHdr; byte m_hdr[16]; @@ -139,6 +141,8 @@ public: const ReliableSendInfo_t &ReliableSendInfo() const { DbgAssert( m_nFlags & k_nSteamNetworkingSend_Reliable ); + // !KLUDGE! Reuse the identity field. We don't actually put this in a union because + // this is internal stuff that doesn't need to be exposed in the API COMPILE_TIME_ASSERT( sizeof(m_identityPeer) >= sizeof(ReliableSendInfo_t) ); return *(ReliableSendInfo_t *)&m_identityPeer; } @@ -412,11 +416,7 @@ struct SSNPSenderState SteamNetworkingMessageQueue m_messagesQueued; /// List of reliable messages that have been fully placed on the wire at least once, - /// but we're hanging onto because of the potential need to retry. (Note that if we get - /// packet loss, it's possible that we hang onto a message even after it's been fully - /// acked, because a prior message is still needed. We always operate on this list - /// like a queue, rather than seeking into the middle of the list and removing messages - /// as soon as they are no longer needed.) + /// but we're hanging onto because of the potential need to retry. SteamNetworkingMessageQueue m_unackedReliableMessages; // Buffered data counters. See SteamNetworkingQuickConnectionStatus for more info