fix: close connection on reliable send queue overflow (#1747)

Author: simon-lemay-unity

com.unity.netcode.adapter.utp/CHANGELOG.md

@@ -9,6 +9,7 @@ All notable changes to this package will be documented in this file. The format
 ### Changed
 
 - Updated Unity Transport package to 1.0.0-pre.13. (#1696)
+- Overflowing the reliable send queue of a connection will now result in the connection being closed, rather than spamming the log with errors about not being able to send a payload. It is deemed better to close the connection than to lose reliable traffic (which could cause all sorts of weird synchronization issues). (#1747)
 
 ### Fixed
 

com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs

@@ -886,9 +886,45 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel
 
             if (!queue.PushMessage(payload))
             {
-                Debug.LogError($"Couldn't add payload of size {payload.Count} to batched send queue. " +
-                    $"Perhaps configured 'Max Send Queue Size' ({m_MaxSendQueueSize}) is too small for workload.");
-                return;
+                if (pipeline == m_ReliableSequencedPipeline)
+                {
+                    // If the message is sent reliably, then we're over capacity and we can't
+                    // provide any reliability guarantees anymore. Disconnect the client since at
+                    // this point they're bound to become desynchronized.
+
+                    var ngoClientId = NetworkManager?.TransportIdToClientId(clientId) ?? clientId;
+                    Debug.LogError($"Couldn't add payload of size {payload.Count} to reliable send queue. " +
+                        $"Closing connection {ngoClientId} as reliability guarantees can't be maintained. " +
+                        $"Perhaps 'Max Send Queue Size' ({m_MaxSendQueueSize}) is too small for workload.");
+
+                    if (clientId == m_ServerClientId)
+                    {
+                        DisconnectLocalClient();
+                    }
+                    else
+                    {
+                        DisconnectRemoteClient(clientId);
+
+                        // DisconnectRemoteClient doesn't notify SDK of disconnection.
+                        InvokeOnTransportEvent(NetcodeNetworkEvent.Disconnect,
+                            clientId,
+                            default(ArraySegment<byte>),
+                            Time.realtimeSinceStartup);
+                    }
+                }
+                else
+                {
+                    // If the message is sent unreliably, we can always just flush everything out
+                    // to make space in the send queue. This is an expensive operation, but a user
+                    // would need to send A LOT of unreliable traffic in one update to get here.
+
+                    m_Driver.ScheduleFlushSend(default).Complete();
+                    SendBatchedMessages(sendTarget, queue);
+
+                    // Don't check for failure. If it still doesn't work, there's nothing we can do
+                    // at this point and the message is lost (it was sent unreliable anyway).
+                    queue.PushMessage(payload);
+                }
             }
         }
 

com.unity.netcode.adapter.utp/Tests/Runtime/TransportTests.cs

@@ -8,7 +8,6 @@
 using System.Text;
 using UnityEngine;
 using UnityEngine.TestTools;
-using Unity.Netcode.UTP.Utilities;
 using static Unity.Netcode.UTP.RuntimeTests.RuntimeTestsHelpers;
 
 namespace Unity.Netcode.UTP.RuntimeTests
@@ -161,44 +160,6 @@ public IEnumerator SendMaximumPayloadSize([ValueSource("k_DeliveryParameters")]
             yield return null;
         }
 
-        [UnityTest]
-        public IEnumerator FilledSendQueueMultipleSends([ValueSource("k_DeliveryParameters")] NetworkDelivery delivery)
-        {
-            InitializeTransport(out m_Server, out m_ServerEvents);
-            InitializeTransport(out m_Client1, out m_Client1Events);
-
-            m_Server.StartServer();
-            m_Client1.StartClient();
-
-            yield return WaitForNetworkEvent(NetworkEvent.Connect, m_Client1Events);
-
-            var numSends = UnityTransport.InitialMaxSendQueueSize / 1024;
-
-            for (int i = 0; i < numSends; i++)
-            {
-                // We remove 4 bytes because each send carries a 4 bytes overhead in the send queue.
-                // Without that we wouldn't fill the send queue; it would get flushed right when we
-                // try to send the last message.
-                var payload = new ArraySegment<byte>(new byte[1024 - BatchedSendQueue.PerMessageOverhead]);
-                m_Client1.Send(m_Client1.ServerClientId, payload, delivery);
-            }
-
-            // Manually wait. This ends up generating quite a bit of packets and it might take a
-            // while for everything to make it to the server.
-            yield return new WaitForSeconds(numSends * 0.02f);
-
-            // Extra event is the connect event.
-            Assert.AreEqual(numSends + 1, m_ServerEvents.Count);
-
-            for (int i = 1; i <= numSends; i++)
-            {
-                Assert.AreEqual(NetworkEvent.Data, m_ServerEvents[i].Type);
-                Assert.AreEqual(1024 - BatchedSendQueue.PerMessageOverhead, m_ServerEvents[i].Data.Count);
-            }
-
-            yield return null;
-        }
-
         // Check making multiple sends to a client in a single frame.
         [UnityTest]
         public IEnumerator MultipleSendsSingleFrame([ValueSource("k_DeliveryParameters")] NetworkDelivery delivery)
@@ -305,6 +266,74 @@ public IEnumerator ReceiveMultipleClients([ValueSource("k_DeliveryParameters")]
 
             yield return null;
         }
+
+        // Check that we get disconnected when overflowing the reliable send queue.
+        [UnityTest]
+        public IEnumerator DisconnectOnReliableSendQueueOverflow()
+        {
+            InitializeTransport(out m_Server, out m_ServerEvents);
+            InitializeTransport(out m_Client1, out m_Client1Events);
+
+            m_Server.StartServer();
+            m_Client1.StartClient();
+
+            yield return WaitForNetworkEvent(NetworkEvent.Connect, m_Client1Events);
+
+            m_Server.Shutdown();
+
+            var numSends = (UnityTransport.InitialMaxSendQueueSize / 1024) + 1;
+
+            for (int i = 0; i < numSends; i++)
+            {
+                var payload = new ArraySegment<byte>(new byte[1024]);
+                m_Client1.Send(m_Client1.ServerClientId, payload, NetworkDelivery.Reliable);
+            }
+
+            LogAssert.Expect(LogType.Error, "Couldn't add payload of size 1024 to reliable send queue. " +
+                $"Closing connection {m_Client1.ServerClientId} as reliability guarantees can't be maintained. " +
+                $"Perhaps 'Max Send Queue Size' ({UnityTransport.InitialMaxSendQueueSize}) is too small for workload.");
+
+            Assert.AreEqual(2, m_Client1Events.Count);
+            Assert.AreEqual(NetworkEvent.Disconnect, m_Client1Events[1].Type);
+
+            yield return null;
+        }
+
+        // Check that it's fine to overflow the unreliable send queue (traffic is flushed on overflow).
+        [UnityTest]
+        public IEnumerator SendCompletesOnUnreliableSendQueueOverflow()
+        {
+            InitializeTransport(out m_Server, out m_ServerEvents);
+            InitializeTransport(out m_Client1, out m_Client1Events);
+
+            m_Server.StartServer();
+            m_Client1.StartClient();
+
+            yield return WaitForNetworkEvent(NetworkEvent.Connect, m_Client1Events);
+
+            var numSends = (UnityTransport.InitialMaxSendQueueSize / 1024) + 1;
+
+            for (int i = 0; i < numSends; i++)
+            {
+                var payload = new ArraySegment<byte>(new byte[1024]);
+                m_Client1.Send(m_Client1.ServerClientId, payload, NetworkDelivery.Unreliable);
+            }
+
+            // Manually wait. This ends up generating quite a bit of packets and it might take a
+            // while for everything to make it to the server.
+            yield return new WaitForSeconds(numSends * 0.02f);
+
+            // Extra event is the connect event.
+            Assert.AreEqual(numSends + 1, m_ServerEvents.Count);
+
+            for (int i = 1; i <= numSends; i++)
+            {
+                Assert.AreEqual(NetworkEvent.Data, m_ServerEvents[i].Type);
+                Assert.AreEqual(1024, m_ServerEvents[i].Data.Count);
+            }
+
+            yield return null;
+        }
     }
 }
 #endif

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -1335,7 +1335,7 @@ private IEnumerator ApprovalTimeout(ulong clientId)
             }
         }
 
-        private ulong TransportIdToClientId(ulong transportId)
+        internal ulong TransportIdToClientId(ulong transportId)
         {
             return transportId == m_ServerTransportId ? ServerClientId : m_TransportIdToClientIdMap[transportId];
         }