fix: client not approved timeout exception

[NoelStephensUnity]

If connection approval is enabled and a client is not approved, on transports other than UnityTransport the server could throw an exception after the time out coroutine stopped. Additionally, timeout coroutines were not being stopped upon a client being approved or not approved. This PR fixes these minor issues with tests.

Based on #2389 discovery that some third party NetworkTransport implementations might immediately invoke a disconnect network event.

Changelog

• Fixed: Issue with approval time out where upon a client not being approved the server-side time out coroutine, when it ended, could throw an exception on third party transports.
• Fixed: Issue where the approval time out coroutine was not being stopped once a client was approved or not approved.

Testing and Documentation

• Includes integration test.
• No documentation changes or additions were necessary.

[0xFA11]

LGTM! 🚀

[0xFA11]

I think this changes the flow — I'll give it a second thought.

[NoelStephensUnity]

I think this changes the flow — I'll give it a second thought.

Please do. Would like to have another set of eyes on this.
Summary of the issue:
With other transports the removal of client id mapping can happen immediately (server side) when the client is not approved yet the coroutine is left running. Upon timing out (a non-approved server-side client time out coroutine), the server then attempts to disconnect the client which ends up throwing an exception. This doesn't happen with UnityTransport (currently) so this PR is to just to track the coroutines and end them when the client or the server side have received some form of approval response.

[0xFA11]

First, I'd like to quote your (@NoelStephensUnity) comment inline here (for the discussion):

With other transports the removal of client id mapping can happen immediately (server side) when the client is not approved yet the coroutine is left running. Upon timing out (a non-approved server-side client time out coroutine), the server then attempts to disconnect the client which ends up throwing an exception. This doesn't happen with UnityTransport (currently) so this PR is to just to track the coroutines and end them when the client or the server side have received some form of approval response.

[0xFA11]

Also quoting an exception from the linked issue (#2389):

System.Collections.Generic.Dictionary`2[TKey,TValue].get_Item (TKey key) (at <d6232873609549b8a045fa15811a5bd3>:0)
Unity.Netcode.NetworkManager.ClientIdToTransportId (System.UInt64 clientId) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:1781)
Unity.Netcode.NetworkManager.DisconnectRemoteClient (System.UInt64 clientId) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:1361)
Unity.Netcode.NetworkManager.DisconnectClient (System.UInt64 clientId, System.String reason) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:2063)
Unity.Netcode.NetworkManager.DisconnectClient (System.UInt64 clientId) (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:2038)
Unity.Netcode.NetworkManager+<ApprovalTimeout>d__180.MoveNext () (at Library/PackageCache/[email protected]/Runtime/Core/NetworkManager.cs:1765)
UnityEngine.SetupCoroutine.InvokeMoveNext (System.Collections.IEnumerator enumerator, System.IntPtr returnValueAddress) (at <bae255e3e08e46f7bc2fbd23dde96338>:0)```

[0xFA11]

I think we should not change the behavior here — the minimum fix is to simply handle NM.DisconnectClient() call gracefully and leave other issues/exceptions related to transport, including 3rd-party ones, up their own implementation.

As a rule of thumb, a transport should be able to handle a disconnect or a send message call to an unknown or "just-removed" connection gracefully.

Perhaps this would give the general idea:

internal ulong TransportIdToClientId(ulong transportId)
{
-    return transportId == m_ServerTransportId ? ServerClientId : m_TransportIdToClientIdMap[transportId];
+    if (transportId == m_ServerTransportId)
+    {
+        return ServerClientId;
+    }
+
+    if (m_TransportIdToClientIdMap.TryGetValue(transportId, out var clientId))
+    {
+        return clientId;
+    }
+
+    return default;
}

internal ulong ClientIdToTransportId(ulong clientId)
{
-    return clientId == ServerClientId ? m_ServerTransportId : m_ClientIdToTransportIdMap[clientId];
+    if (clientId == ServerClientId)
+    {
+        return m_ServerTransportId;
+    }
+
+    if (m_ClientIdToTransportIdMap.TryGetValue(clientId, out var transportId))
+    {
+        return transportId;
+    }
+
+    return default;
}

[NoelStephensUnity]

Yeah, not sure why I approached it this way.
Fixing the transport id mapping is indeed a less intrusive fix for the issue.

[NoelStephensUnity]

Added the suggested changes to #2475 and closing this PR.