From ab5ef56aa2cfadf897c8977695436ffb8d8d3ad0 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Fri, 5 Feb 2021 18:07:07 -0800 Subject: [PATCH] Add k_ESteamNetworkingConfig_ConnectionUserData Now we store the userdata in a config value, so that it can be set atomically when a connection is created, and also so that the default value of -1 can be customized. Also added some warnings about the dangers of using the userData field in callback structs. I worry I've created a footgun here, and would be tempted to remove the field entirely from SteamNetConnectionStatusChangedCallback_t, but it is coming in through a member struct SteamNetConnectionInfo_t, and it makes sense there. Addresses problems discussed in issue #162. P4:6354936 --- include/steam/isteamnetworkingsockets.h | 18 ++++- include/steam/steamnetworkingtypes.h | 35 +++++++++ .../clientlib/csteamnetworkingsockets.cpp | 76 +++++++++++++------ .../steamnetworkingsockets_connections.cpp | 12 +-- .../steamnetworkingsockets_connections.h | 10 ++- .../steamnetworkingsockets_internal.h | 1 + 6 files changed, 119 insertions(+), 33 deletions(-) diff --git a/include/steam/isteamnetworkingsockets.h b/include/steam/isteamnetworkingsockets.h index 685edcd..b286a25 100644 --- a/include/steam/isteamnetworkingsockets.h +++ b/include/steam/isteamnetworkingsockets.h @@ -192,7 +192,23 @@ public: /// Set connection user data. the data is returned in the following places /// - You can query it using GetConnectionUserData. /// - The SteamNetworkingmessage_t structure. - /// - The SteamNetConnectionInfo_t structure. (Which is a member of SteamNetConnectionStatusChangedCallback_t.) + /// - The SteamNetConnectionInfo_t structure. + /// (Which is a member of SteamNetConnectionStatusChangedCallback_t -- but see WARNINGS below!!!!) + /// + /// Do you need to set this atomically when the connection is created? + /// See k_ESteamNetworkingConfig_ConnectionUserData. + /// + /// WARNING: Be *very careful* when using the value provided in callbacks structs. + /// Callbacks are queued, and the value that you will receive in your + /// callback is the userdata that was effective at the time the callback + /// was queued. There are subtle race conditions that can hapen if you + /// don't understand this! + /// + /// If any incoming messages for this connection are queued, the userdata + /// field is updated, so that when when you receive messages (e.g. with + /// ReceiveMessagesOnConnection), they will always have the very latest + /// userdata. So the tricky race conditions that can happen with callbacks + /// do not apply to retrieving messages. /// /// Returns false if the handle is invalid. virtual bool SetConnectionUserData( HSteamNetConnection hPeer, int64 nUserData ) = 0; diff --git a/include/steam/steamnetworkingtypes.h b/include/steam/steamnetworkingtypes.h index 4d035bf..00465e1 100644 --- a/include/steam/steamnetworkingtypes.h +++ b/include/steam/steamnetworkingtypes.h @@ -1129,6 +1129,41 @@ enum ESteamNetworkingConfigValue /// Default is 512k (524288 bytes) k_ESteamNetworkingConfig_SendBufferSize = 9, + /// [connection int64] Get/set userdata as a configuration option. + /// The default value is -1. You may want to set the user data as + /// a config value, instead of using ISteamNetworkingSockets::SetConnectionUserData + /// in two specific instances: + /// + /// - You wish to set the userdata atomically when creating + /// an outbound connection, so that the userdata is filled in properly + /// for any callbacks that happen. However, note that this trick + /// only works for connections initiated locally! For incoming + /// connections, multiple state transitions may happen and + /// callbacks be queued, before you are able to service the first + /// callback! Be careful! + /// + /// - You can set the default userdata for all newly created connections + /// by setting this value at a higher level (e.g. on the listen + /// socket or at the global level.) Then this default + /// value will be inherited when the connection is created. + /// This is useful in case -1 is a valid userdata value, and you + /// wish to use something else as the default value so you can + /// tell if it has been set or not. + /// + /// HOWEVER: once a connection is created, the effective value is + /// then bound to the connection. Unlike other connection options, + /// if you change it again at a higher level, the new value will not + /// be inherited by connections. + /// + /// Using the userdata field in callback structs is not advised because + /// of tricky race conditions. Instead, you might try one of these methods: + /// + /// - Use a separate map with the HSteamNetConnection as the key. + /// - Fetch the userdata from the connection in your callback + /// using ISteamNetworkingSockets::GetConnectionUserData, to + // ensure you have the current value. + k_ESteamNetworkingConfig_ConnectionUserData = 40, + /// [connection int32] Minimum/maximum send rate clamp, 0 is no limit. /// This value will control the min/max allowed sending rate that /// bandwidth estimation is allowed to reach. Default is 0 (no-limit) diff --git a/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp b/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp index e16f482..37058e1 100644 --- a/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp +++ b/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp @@ -58,6 +58,7 @@ DEFINE_GLOBAL_CONFIGVAL( void *, Callback_CreateConnectionSignaling, nullptr ); DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, TimeoutInitial, 10000, 0, INT32_MAX ); DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, TimeoutConnected, 10000, 0, INT32_MAX ); DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, SendBufferSize, 512*1024, 0, 0x10000000 ); +DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int64, ConnectionUserData, -1 ); // no limits here DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, SendRateMin, 128*1024, 1024, 0x10000000 ); DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, SendRateMax, 1024*1024, 1024, 0x10000000 ); DEFINE_CONNECTON_DEFAULT_CONFIGVAL( int32, NagleTime, 5000, 0, 20000 ); @@ -1459,12 +1460,12 @@ static ConfigValue *EvaluateScopeConfigValue( GlobalConfigValueEntry *pEntry, return nullptr; } -static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) +static bool AssignConfigValueTyped( int32 *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) { switch ( eDataType ) { case k_ESteamNetworkingConfig_Int32: - pVal->m_data = *(int32*)pArg; + *pVal = *(int32*)pArg; break; case k_ESteamNetworkingConfig_Int64: @@ -1472,12 +1473,12 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo int64 arg = *(int64*)pArg; if ( (int32)arg != arg ) return false; // Cannot truncate! - pVal->m_data = *(int32*)arg; + *pVal = *(int32*)arg; break; } case k_ESteamNetworkingConfig_Float: - pVal->m_data = (int32)floor( *(float*)pArg + .5f ); + *pVal = (int32)floor( *(float*)pArg + .5f ); break; case k_ESteamNetworkingConfig_String: @@ -1485,7 +1486,7 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo int x; if ( sscanf( (const char *)pArg, "%d", &x ) != 1 ) return false; - pVal->m_data = x; + *pVal = x; break; } @@ -1496,22 +1497,22 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo return true; } -static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) +static bool AssignConfigValueTyped( int64 *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) { switch ( eDataType ) { case k_ESteamNetworkingConfig_Int32: - pVal->m_data = *(int32*)pArg; + *pVal = *(int32*)pArg; break; case k_ESteamNetworkingConfig_Int64: { - pVal->m_data = *(int64*)pArg; + *pVal = *(int64*)pArg; break; } case k_ESteamNetworkingConfig_Float: - pVal->m_data = (int64)floor( *(float*)pArg + .5f ); + *pVal = (int64)floor( *(float*)pArg + .5f ); break; case k_ESteamNetworkingConfig_String: @@ -1519,7 +1520,7 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo long long x; if ( sscanf( (const char *)pArg, "%lld", &x ) != 1 ) return false; - pVal->m_data = (int64)x; + *pVal = (int64)x; break; } @@ -1530,22 +1531,22 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo return true; } -static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) +static bool AssignConfigValueTyped( float *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) { switch ( eDataType ) { case k_ESteamNetworkingConfig_Int32: - pVal->m_data = (float)( *(int32*)pArg ); + *pVal = (float)( *(int32*)pArg ); break; case k_ESteamNetworkingConfig_Int64: { - pVal->m_data = (float)( *(int64*)pArg ); + *pVal = (float)( *(int64*)pArg ); break; } case k_ESteamNetworkingConfig_Float: - pVal->m_data = *(float*)pArg; + *pVal = *(float*)pArg; break; case k_ESteamNetworkingConfig_String: @@ -1553,7 +1554,7 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo float x; if ( sscanf( (const char *)pArg, "%f", &x ) != 1 ) return false; - pVal->m_data = x; + *pVal = x; break; } @@ -1564,7 +1565,7 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingCo return true; } -static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) +static bool AssignConfigValueTyped( std::string *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) { char temp[64]; @@ -1572,21 +1573,21 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetwor { case k_ESteamNetworkingConfig_Int32: V_sprintf_safe( temp, "%d", *(int32*)pArg ); - pVal->m_data = temp; + *pVal = temp; break; case k_ESteamNetworkingConfig_Int64: V_sprintf_safe( temp, "%lld", (long long)*(int64*)pArg ); - pVal->m_data = temp; + *pVal = temp; break; case k_ESteamNetworkingConfig_Float: V_sprintf_safe( temp, "%g", *(float*)pArg ); - pVal->m_data = temp; + *pVal = temp; break; case k_ESteamNetworkingConfig_String: - pVal->m_data = (const char *)pArg; + *pVal = (const char *)pArg; break; default: @@ -1596,12 +1597,12 @@ static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetwor return true; } -static bool AssignConfigValueTyped( ConfigValue *pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) +static bool AssignConfigValueTyped( void **pVal, ESteamNetworkingConfigDataType eDataType, const void *pArg ) { switch ( eDataType ) { case k_ESteamNetworkingConfig_Ptr: - pVal->m_data = *(void **)pArg; + *pVal = *(void **)pArg; break; default: @@ -1637,6 +1638,12 @@ bool SetConfigValueTyped( Assert( pGlobal->IsSet() ); pGlobal->m_data = pGlobal->m_defaultValue; } + else if ( eScopeType == k_ESteamNetworkingConfig_Connection && pEntry->m_eValue == k_ESteamNetworkingConfig_ConnectionUserData ) + { + // Once this is set, we cannot clear it or inherit it. + SpewError( "Cannot clear connection user data\n" ); + return false; + } else { Assert( pVal->m_pInherit ); @@ -1646,7 +1653,7 @@ bool SetConfigValueTyped( } // Call type-specific method to set it - if ( !AssignConfigValueTyped( pVal, eDataType, pArg ) ) + if ( !AssignConfigValueTyped( &pVal->m_data, eDataType, pArg ) ) return false; // Mark it as set @@ -1736,6 +1743,29 @@ bool CSteamNetworkingUtils::SetConfigValue( ESteamNetworkingConfigValue eValue, case k_ESteamNetworkingConfig_MTU_DataSize: SpewWarning( "MTU_DataSize is readonly" ); return false; + + case k_ESteamNetworkingConfig_ConnectionUserData: + { + + // We only need special handling when modifying a connection + if ( eScopeType != k_ESteamNetworkingConfig_Connection ) + break; + + // Process the user argument, maybe performing type conversion + int64 newData; + if ( !AssignConfigValueTyped( &newData, eDataType, pValue ) ) + return false; + + // Lookup the connection + CSteamNetworkConnectionBase *pConn = GetConnectionByHandle( HSteamNetConnection( scopeObj ) ); + if ( !pConn ) + return false; + + // Set the data, possibly fixing up existing queued messages, etc + pConn->SetUserData( pConn->m_connectionConfig.m_ConnectionUserData.m_data ); + return true; + } + } GlobalConfigValueEntry *pEntry = FindConfigValueEntry( eValue ); diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp index 0711988..fa8bc6f 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp @@ -615,7 +615,6 @@ CSteamNetworkConnectionBase::CSteamNetworkConnectionBase( CSteamNetworkingSocket : m_pSteamNetworkingSocketsInterface( pSteamNetworkingSocketsInterface ) { m_hConnectionSelf = k_HSteamNetConnection_Invalid; - m_nUserData = -1; m_eConnectionState = k_ESteamNetworkingConnectionState_None; m_eConnectionWireState = k_ESteamNetworkingConnectionState_None; m_usecWhenEnteredConnectionState = 0; @@ -943,6 +942,9 @@ bool CSteamNetworkConnectionBase::BInitConnection( SteamNetworkingMicroseconds u return false; } + // Bind effective user data into the connection now. It can no longer be inherited + m_connectionConfig.m_ConnectionUserData.Set( m_connectionConfig.m_ConnectionUserData.Get() ); + // Make sure a description has been set for debugging purposes SetDescription(); @@ -1698,7 +1700,7 @@ ESteamNetConnectionEnd CSteamNetworkConnectionBase::CheckRemoteCert( const CertA void CSteamNetworkConnectionBase::SetUserData( int64 nUserData ) { - m_nUserData = nUserData; + m_connectionConfig.m_ConnectionUserData.Set( nUserData ); // Change user data on all messages that haven't been pulled out // of the queue yet. This way we don't expose the client to weird @@ -1707,7 +1709,7 @@ void CSteamNetworkConnectionBase::SetUserData( int64 nUserData ) for ( CSteamNetworkingMessage *m = m_queueRecvMessages.m_pFirst ; m ; m = m->m_links.m_pNext ) { Assert( m->m_conn == m_hConnectionSelf ); - m->m_nConnUserData = m_nUserData; + m->m_nConnUserData = nUserData; } } @@ -1743,7 +1745,7 @@ void CSteamNetworkConnectionBase::ConnectionPopulateInfo( SteamNetConnectionInfo info.m_eState = CollapseConnectionStateToAPIState( m_eConnectionState ); info.m_hListenSocket = m_pParentListenSocket ? m_pParentListenSocket->m_hListenSocketSelf : k_HSteamListenSocket_Invalid; info.m_identityRemote = m_identityRemote; - info.m_nUserData = m_nUserData; + info.m_nUserData = GetUserData(); info.m_eEndReason = m_eEndReason; V_strcpy_safe( info.m_szEndDebug, m_szEndDebug ); V_strcpy_safe( info.m_szConnectionDescription, m_szDescription ); @@ -3314,7 +3316,7 @@ int64 CSteamNetworkConnectionPipe::_APISendMessageToConnection( CSteamNetworking pMsg->m_nMessageNumber = nMsgNum; pMsg->m_conn = m_pPartner->m_hConnectionSelf; pMsg->m_identityPeer = m_pPartner->m_identityRemote; - pMsg->m_nConnUserData = m_pPartner->m_nUserData; + pMsg->m_nConnUserData = m_pPartner->GetUserData(); pMsg->m_usecTimeReceived = usecNow; // Pass directly to our partner diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h index 317e020..e4a9f61 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h @@ -315,7 +315,12 @@ public: // // Get/set user data - inline int64 GetUserData() const { return m_nUserData; } + inline int64 GetUserData() const + { + // User data is locked when we create a connection! + Assert( m_connectionConfig.m_ConnectionUserData.IsSet() ); + return m_connectionConfig.m_ConnectionUserData.m_data; + } void SetUserData( int64 nUserData ); // Get/set name @@ -574,9 +579,6 @@ protected: /// Called from BInitConnection, to start obtaining certs, etc virtual void InitConnectionCrypto( SteamNetworkingMicroseconds usecNow ); - /// User data - int64 m_nUserData; - /// Name assigned by app (for debugging) char m_szAppName[ k_cchSteamNetworkingMaxConnectionDescription ]; diff --git a/src/steamnetworkingsockets/steamnetworkingsockets_internal.h b/src/steamnetworkingsockets/steamnetworkingsockets_internal.h index 9072fec..6b97674 100644 --- a/src/steamnetworkingsockets/steamnetworkingsockets_internal.h +++ b/src/steamnetworkingsockets/steamnetworkingsockets_internal.h @@ -723,6 +723,7 @@ struct ConnectionConfig ConfigValue m_Unencrypted; ConfigValue m_SymmetricConnect; ConfigValue m_LocalVirtualPort; + ConfigValue m_ConnectionUserData; ConfigValue m_LogLevel_AckRTT; ConfigValue m_LogLevel_PacketDecode;