Refactor how we populate the inline "stats".

To send a data packet, first we ask the transport-specific code to
prepare a context, and figure out how much space it needs to reserve.
Then it passes that into the generic SNP code, which lays out the packet.
Then SNP calls the transport code to actually send the encrypted data,
providing the context that the transport layer originally provided.

This simplified a bunch of stuff and allowed a bunch of cleanup
(especially in the code for relayed connections -- not visible here).
Most importantly (and the reason I realized I needed to refactor this),
it facalitated fixing a serious P2P transport bug, that would have been
awkward to fix without the refactor.

Author: zpostfacto

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp

@@ -11,6 +11,12 @@
 // memdbgon must be the last include file in a .cpp file!!!
 #include "tier0/memdbgon.h"
 
+#ifdef __GNUC__
+	// error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow]
+	// current steamrt:scout gcc "g++ (SteamRT 4.8.4-1ubuntu15~12.04+steamrt1.2+srt1) 4.8.4" requires this at the top due to optimizations
+	#pragma GCC diagnostic ignored "-Wstrict-overflow"
+#endif
+
 // Put everything in a namespace, so we don't violate the one definition rule
 namespace SteamNetworkingSocketsLib {
 
@@ -1908,12 +1914,19 @@ void CSteamNetworkConnectionBase::CheckConnectionStateAndSetNextThinkTime( Steam
 		// V
 		case k_ESteamNetworkingConnectionState_Connected:
 		{
-			SteamNetworkingMicroseconds usecNextThinkSNP = SNP_ThinkSendState( usecNow );
-			AssertMsg1( usecNextThinkSNP > usecNow, "SNP next think time must be in in the future.  It's %lldusec in the past", (long long)( usecNow - usecNextThinkSNP ) );
+			if ( BCanSendEndToEndData() )
+			{
+				SteamNetworkingMicroseconds usecNextThinkSNP = SNP_ThinkSendState( usecNow );
+				AssertMsg1( usecNextThinkSNP > usecNow, "SNP next think time must be in in the future.  It's %lldusec in the past", (long long)( usecNow - usecNextThinkSNP ) );
 
-			// Set a pretty tight tolerance if SNP wants to wake up at a certain time.
-			if ( usecNextThinkSNP < k_nThinkTime_Never )
-				UpdateMinThinkTime( usecNextThinkSNP, +1 );
+				// Set a pretty tight tolerance if SNP wants to wake up at a certain time.
+				if ( usecNextThinkSNP < k_nThinkTime_Never )
+					UpdateMinThinkTime( usecNextThinkSNP, +1 );
+			}
+			else
+			{
+				UpdateMinThinkTime( usecNow + 20*1000, +5 );
+			}
 		} break;
 	}
 
@@ -2247,7 +2260,13 @@ EResult CSteamNetworkConnectionPipe::APIAcceptConnection()
 	return k_EResultFail;
 }
 
-int CSteamNetworkConnectionPipe::SendEncryptedDataChunk( const void *pChunk, int cbChunk, SteamNetworkingMicroseconds usecNow, void *pConnectionContext )
+bool CSteamNetworkConnectionPipe::SendDataPacket( SteamNetworkingMicroseconds usecNow )
+{
+	AssertMsg( false, "CSteamNetworkConnectionPipe connections shouldn't try to send 'packets'!" );
+	return false;
+}
+
+int CSteamNetworkConnectionPipe::SendEncryptedDataChunk( const void *pChunk, int cbChunk, SendPacketContext_t &ctx )
 {
 	AssertMsg( false, "CSteamNetworkConnectionPipe connections shouldn't try to send 'packets'!" );
 	return -1;

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h

@@ -73,6 +73,68 @@ struct RemoteConnectionKey_t
 	}
 };
 
+/// Base class for connection-type-specific context structure 
+struct SendPacketContext_t
+{
+	inline SendPacketContext_t( SteamNetworkingMicroseconds usecNow ) : m_usecNow( usecNow ) {}
+	const SteamNetworkingMicroseconds m_usecNow;
+	int m_cbMaxEncryptedPayload;
+};
+
+template<typename TStatsMsg>
+struct SendPacketContext : SendPacketContext_t
+{
+	inline SendPacketContext( SteamNetworkingMicroseconds usecNow ) : SendPacketContext_t( usecNow ) {}
+
+	uint32 m_nFlags; // Message flags that we need to set.
+	TStatsMsg msg; // Type-specific stats message
+	int m_cbMsgSize; // Size of message
+	int m_cbTotalSize; // Size needed in the header, including the serialized size field
+
+	void SlamFlagsAndCalcSize()
+	{
+		SetStatsMsgFlagsIfNotImplied( msg, m_nFlags );
+		m_cbTotalSize = m_cbMsgSize = msg.ByteSize();
+		if ( m_cbMsgSize > 0 )
+			m_cbTotalSize += VarIntSerializedSize( (uint32)m_cbMsgSize );
+	}
+
+	bool Serialize( byte *&p )
+	{
+		if ( m_cbTotalSize <= 0 )
+			return false;
+
+		// Serialize the stats size, var-int encoded
+		byte *pOut = SerializeVarInt( p, uint32( m_cbMsgSize ) );
+
+		// Serialize the actual message
+		pOut = msg.SerializeWithCachedSizesToArray( pOut );
+
+		// Make sure we wrote the number of bytes we expected
+		if ( pOut != p + m_cbTotalSize )
+		{
+			// ABORT!
+			AssertMsg( false, "Size mismatch after serializing inline stats blob" );
+			return false;
+		}
+
+		// Advance pointer
+		p = pOut;
+		return true;
+	}
+
+	void CalcMaxEncryptedPayloadSize( size_t cbHdrReserve )
+	{
+		Assert( m_cbTotalSize >= 0 );
+		m_cbMaxEncryptedPayload = k_cbSteamNetworkingSocketsMaxUDPMsgLen - (int)cbHdrReserve - m_cbTotalSize;
+		if ( m_cbMaxEncryptedPayload < 512 )
+		{
+			AssertMsg2( m_cbMaxEncryptedPayload < 512, "%s is really big (%d bytes)!", msg.GetTypeName().c_str(), m_cbTotalSize );
+		}
+	}
+
+};
+
 /////////////////////////////////////////////////////////////////////////////
 //
 // Message storage implementation
@@ -379,8 +441,14 @@ class CSteamNetworkConnectionBase : protected IThinker
 		return m_receiverState.TimeWhenFlushAcks() < INT64_MAX || SNP_TimeWhenWantToSendNextPacket() < INT64_MAX;
 	}
 
-	/// Send a data packet now, even if we don't have the bandwidth available
-	int SNP_SendPacket( SteamNetworkingMicroseconds usecNow, int cbMaxEncryptedPayload, void *pConnectionData );
+	/// Called by SNP pacing layer, when it has some data to send and there is bandwidth available.
+	/// The derived class should setup a context, reserving the space it needs, and then call SNP_SendPacket.
+	/// Returns true if a packet was sent successfuly, false if there was a problem.
+	virtual bool SendDataPacket( SteamNetworkingMicroseconds usecNow ) = 0;
+
+	/// Send a data packet now, even if we don't have the bandwidth available.  Returns true if a packet was
+	/// sent successfully, false if there was a problem.  This will call SendEncryptedDataChunk to do the work
+	bool SNP_SendPacket( SendPacketContext_t &ctx );
 
 	// Steam memory accounting
 	#ifdef DBGFLAG_VALIDATE
@@ -454,7 +522,7 @@ class CSteamNetworkConnectionBase : protected IThinker
 	/// pConnectionContext is whatever the connection later passed
 	/// to SNP_SendPacket, if the connection initiated the sending
 	/// of the packet
-	virtual int SendEncryptedDataChunk( const void *pChunk, int cbChunk, SteamNetworkingMicroseconds usecNow, void *pConnectionContext ) = 0;
+	virtual int SendEncryptedDataChunk( const void *pChunk, int cbChunk, SendPacketContext_t &ctx ) = 0;
 
 	/// Called when we receive a complete message.  Should allocate a message object and put it into the proper queues
 	void ReceivedMessage( const void *pData, int cbData, int64 nMsgNum, SteamNetworkingMicroseconds usecNow );
@@ -632,7 +700,8 @@ class CSteamNetworkConnectionPipe : public CSteamNetworkConnectionBase
 	virtual void SendEndToEndConnectRequest( SteamNetworkingMicroseconds usecNow ) OVERRIDE;
 	virtual void SendEndToEndPing( bool bUrgent, SteamNetworkingMicroseconds usecNow ) OVERRIDE;
 	virtual EResult APIAcceptConnection() OVERRIDE;
-	virtual int SendEncryptedDataChunk( const void *pChunk, int cbChunk, SteamNetworkingMicroseconds usecNow, void *pConnectionContext ) OVERRIDE;
+	virtual bool SendDataPacket( SteamNetworkingMicroseconds usecNow ) OVERRIDE;
+	virtual int SendEncryptedDataChunk( const void *pChunk, int cbChunk, SendPacketContext_t &ctx ) OVERRIDE;
 	virtual EResult _APISendMessageToConnection( const void *pData, uint32 cbData, int nSendFlags ) OVERRIDE;
 	virtual void ConnectionStateChanged( ESteamNetworkingConnectionState eOldState ) OVERRIDE;
 	virtual void PostConnectionStateChangedCallback( ESteamNetworkingConnectionState eOldAPIState, ESteamNetworkingConnectionState eNewAPIState ) OVERRIDE;

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp

@@ -1299,20 +1299,18 @@ inline bool HasOverlappingRange( const SNPRange_t &range, const std::map<SNPRang
 	return false;
 }
 
-int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds usecNow, int cbMaxEncryptedPayload, void *pConnectionData )
+bool CSteamNetworkConnectionBase::SNP_SendPacket( SendPacketContext_t &ctx )
 {
-	// If we aren't being specifically asked to send a packet, and we don't have anything to send,
-	// then don't send right now.
-	if ( pConnectionData == nullptr && usecNow < m_receiverState.TimeWhenFlushAcks() && usecNow < SNP_TimeWhenWantToSendNextPacket() )
-		return 0;
-
 	// Make sure we have initialized the connection
 	Assert( BStateIsConnectedForWirePurposes() );
 	Assert( !m_senderState.m_mapInFlightPacketsByPktNum.empty() );
 
+	SteamNetworkingMicroseconds usecNow = ctx.m_usecNow;
+
 	// Get max size of plaintext we could send.
 	// AES-GCM has a fixed size overhead, for the tag.
-	int cbMaxPlaintextPayload = std::max( 0, cbMaxEncryptedPayload-k_cbSteamNetwokingSocketsEncrytionTagSize );
+	int cbMaxPlaintextPayload = std::max( 0, ctx.m_cbMaxEncryptedPayload-k_cbSteamNetwokingSocketsEncrytionTagSize );
+	cbMaxPlaintextPayload = std::min( cbMaxPlaintextPayload, k_cbSteamNetworkingSocketsMaxPlaintextPayloadSend );
 
 	uint8 payload[ k_cbSteamNetworkingSocketsMaxPlaintextPayloadSend ];
 	uint8 *pPayloadEnd = payload + cbMaxPlaintextPayload;
@@ -1326,7 +1324,7 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 	// Stop waiting frame
 	pPayloadPtr = SNP_SerializeStopWaitingFrame( pPayloadPtr, pPayloadEnd, usecNow );
 	if ( pPayloadPtr == nullptr )
-		return -1;
+		return false;
 
 	// Get list of ack blocks we might want to serialize, and which
 	// of those acks we really want to flush out right now.
@@ -1373,11 +1371,8 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 	}
 
 	// Check if we don't actually have bandwidth to send data, then don't.
-	// (This means that acks or other responsibilities are choking the pipe
-	// and should basically never happen in ordinary circumstances!)
 	if ( m_senderState.m_flTokenBucket < 0.0 )
 	{
-		SpewWarningRateLimited( usecNow, "[%s] Exceeding rate limit just sending acks / stats!  Not sending any data!  (Ack blocks=%d prio=%d)", GetDescription(), ackHelper.m_nBlocks, ackHelper.m_nBlocksNeedToAck );
 
 		// Serialize some acks, if we want to
 		if ( cbReserveForAcks > 0 )
@@ -1386,7 +1381,7 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 			// not just the bare minimum.
 			pPayloadPtr = SNP_SerializeAckBlocks( ackHelper, pPayloadPtr, pPayloadEnd, usecNow );
 			if ( pPayloadPtr == nullptr )
-				return -1; // bug!  Abort
+				return false; // bug!  Abort
 
 			// We don't need to serialize any more acks
 			cbReserveForAcks = 0;
@@ -1597,7 +1592,7 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 
 		uint8 *pAfterAcks = SNP_SerializeAckBlocks( ackHelper, pPayloadPtr, pAckEnd, usecNow );
 		if ( pAfterAcks == nullptr )
-			return -1; // bug!  Abort
+			return false; // bug!  Abort
 
 		int cbAckBytesWritten = pAfterAcks - pPayloadPtr;
 		if ( cbAckBytesWritten > cbReserveForAcks )
@@ -1760,9 +1755,9 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 	//SpewMsg( "Send encrypt IV %llu + %02x%02x%02x%02x, key %02x%02x%02x%02x\n", *(uint64 *)&m_cryptIVSend.m_buf, m_cryptIVSend.m_buf[8], m_cryptIVSend.m_buf[9], m_cryptIVSend.m_buf[10], m_cryptIVSend.m_buf[11], m_cryptKeySend.m_buf[0], m_cryptKeySend.m_buf[1], m_cryptKeySend.m_buf[2], m_cryptKeySend.m_buf[3] );
 
 	// Connection-specific method to send it
-	int nBytesSent = SendEncryptedDataChunk( arEncryptedChunk, cbEncrypted, usecNow, pConnectionData );
+	int nBytesSent = SendEncryptedDataChunk( arEncryptedChunk, cbEncrypted, ctx );
 	if ( nBytesSent <= 0 )
-		return -1;
+		return false;
 
 	// We sent a packet.  Track it
 	auto pairInsertResult = m_senderState.m_mapInFlightPacketsByPktNum.insert( pairInsert );
@@ -1782,7 +1777,7 @@ int CSteamNetworkConnectionBase::SNP_SendPacket( SteamNetworkingMicroseconds use
 
 	// We spent some tokens
 	m_senderState.m_flTokenBucket -= (float)nBytesSent;
-	return nBytesSent;
+	return true;
 }
 
 void CSteamNetworkConnectionBase::SNP_GatherAckBlocks( SNPAckSerializerHelper &helper, SteamNetworkingMicroseconds usecNow )
@@ -2950,17 +2945,8 @@ SteamNetworkingMicroseconds CSteamNetworkConnectionBase::SNP_ThinkSendState( Ste
 			return usecNow + 1000;
 		}
 
-		int nBytesSent = SNP_SendPacket( usecNow, k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend, nullptr );
-		if ( nBytesSent < 0 )
-		{
-			// Problem sending packet.  Nuke token bucket, but request
-			// a wakeup relatively quick to check on our state again
-			m_senderState.m_flTokenBucket = m_senderState.m_n_x * -0.001f;
-			return usecNow + 2000;
-		}
-
-		// Nothing to send at this time
-		if ( nBytesSent == 0 )
+		// Check if we have anything to send.
+		if ( usecNow < m_receiverState.TimeWhenFlushAcks() && usecNow < SNP_TimeWhenWantToSendNextPacket() )
 		{
 
 			// We've sent everything we want to send.  Limit our reserve to a
@@ -2970,6 +2956,15 @@ SteamNetworkingMicroseconds CSteamNetworkConnectionBase::SNP_ThinkSendState( Ste
 			break;
 		}
 
+		// Send the next data packet.
+		if ( !SendDataPacket( usecNow ) )
+		{
+			// Problem sending packet.  Nuke token bucket, but request
+			// a wakeup relatively quick to check on our state again
+			m_senderState.m_flTokenBucket = m_senderState.m_n_x * -0.001f;
+			return usecNow + 2000;
+		}
+
 		// We spent some tokens, do we have any left?
 		if ( m_senderState.m_flTokenBucket < 0.0f )
 			break;