Asking for clarification in Chat example

[darksylinc]

Hi!

I've started integrating GNS into a pet project to analyze it, and the following felt wrong:

In ChatServer::OnSteamNetConnectionStatusChanged the code does the following:

case k_ESteamNetworkingConnectionState_ClosedByPeer:
case k_ESteamNetworkingConnectionState_ProblemDetectedLocally:
if ( pInfo->m_eOldState == k_ESteamNetworkingConnectionState_Connected )
{
   m_mapClients.erase( itClient );
}
else
{
    assert( pInfo->m_eOldState == k_ESteamNetworkingConnectionState_Connecting );
}
break;

However looking down in the same switch:

case k_ESteamNetworkingConnectionState_Connecting:
// Add them to the client list, using std::map wacky syntax
m_mapClients[ pInfo->m_hConn ];

What's weird here is that an entry in m_mapClients gets inserted after receiving the k_ESteamNetworkingConnectionState_Connecting message.

However upon disconnection, (i.e. _ClosedByPeer & _ProblemDetectedLocally) it assumes that if previous state was _Connecting then there should be no entry in m_mapClients.

This sounds contradictory.

I can think of the following cases:

1. This is a bug. The code should always remove the entry from m_mapClients
2. This is a bug. The code has no way to know if the closure happened before or after GNS performed a callback with _Connecting. We should first check if there is an entry in m_mapClients, and if there is one, remove it
3. This is expected. _ClosedByPeer / _ProblemDetectedLocally getting called after _Connecting but before _Connected means that GNS never got the chance to call our callback

I suspect this is likely a bug, which would make sense since the opportunity window for triggering it is ridiculously low (it needs to disconnect after _Connecting but before anything else, or perhaps due to how GNS works internally, that situation can't actually happen).

[zpostfacto]

Yeah that looks like a bug, and the situation you describe (such as a transition direction from "connecting" to "problem detected locally") is possible. the code probably shouldn't check the state of the connection, it should just check if there is an existing entry in the map.

[proof88]

Hello,

I also noticed something: sometimes when my server app closes down connection with clients, onSteamNetConnectionStatusChanged() is not invoked with pInfo->m_info.m_eState being as k_ESteamNetworkingConnectionState_ClosedByPeer or k_ESteamNetworkingConnectionState_ProblemDetectedLocally but it gets invoked with k_ESteamNetworkingConnectionState_None and pInfo->m_eOldState is actually k_ESteamNetworkingConnectionState_Connected in that case, so it is a different kind of state transition.
So I simply changed that code to look like this:

void onSteamNetConnectionStatusChanged(SteamNetConnectionStatusChangedCallback_t* pInfo) {
switch (pInfo->m_info.m_eState)
    {
    case k_ESteamNetworkingConnectionState_None:
        // sometimes we fall here immediately from k_ESteamNetworkingConnectionState_Connected when closing down connection
        // fall-through
    case k_ESteamNetworkingConnectionState_ClosedByPeer:
        // fall-through
    case k_ESteamNetworkingConnectionState_ProblemDetectedLocally:
    {
        // Ignore if they were not previously connected.  (If they disconnected
        // before we accepted the connection.)
        if (pInfo->m_eOldState == k_ESteamNetworkingConnectionState_Connected)
        { ...