fix: Sends failing on UTP adapter when queue is full (#1317)

* fix: Sends failing on UTP adapter when queue is full

Or close to full. There were two issues here.

First, the payload capacity of the fragmentation pipeline didn't account
for the entire size of the packet. Unfortunately, payload capacity in
the context of the fragmentation pipeline is a bit of a misnomer since
it also includes headers and other such overhead. We were setting it to
the size of a full send queue, and so when you added UTP headers, we
ended up with a packet too large for the pipeline. Bumping up the value
a bit solves this issue.

Second, when sending a single message that was within 4 bytes of the
send queue capacity, we'd silently fail to send it. The reason was we
didn't account for the extra overhead taken by the data length in the
send queue (which requires 4 bytes). Thus the send code would think the
message could fit in the queue when it couldn't. Fixing this simply
required accounting for the 4 extra bytes in the send logic.

Two tests were also added to test sends when the send queue is full
(either from one big send or from multiple smaller ones in a row). Both
tests fail without the fixes in the adapter.

* Cleaner fix for the send queue being full scenario

This one depends on a fix in UTP that's still being reviewed, though.

* Fix possible re-ordering issue for large messages

* Reword the warning when sending immediately

Events can't be larger than m_SendQueueBatchSize. UTP will drop them and
log an error in the current configuration. Reword the warning so that we
don't imply that it's been sent, only that we're trying to send it. If
it fails, there will be an error in the log following it, making it
clear to the user what happened.

* Update UTP dependency to 1.0.0-pre.7

* fix: Fixing validation error

Co-authored-by: Andrew Spiering <andrews@unity3d.com>

Author: simon-lemay-unity

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

@@ -1,6 +1,16 @@
 # Changelog
 All notable changes to this package will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 
+## [1.0.0-pre.3] - 2021-10-22
+
+### Changed
+
+- Updated Unity Transport package to 1.0.0-pre.7
+
+### Fixed
+
+- Fixed sends failing when send queue is filled or close to be filled.
+
 ## [1.0.0-pre.2] - 2020-12-20
 
 ### Changed

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

@@ -563,10 +563,13 @@ public override void Initialize()
 
             m_NetworkParameters = new List<INetworkParameter>();
 
-            // If we want to be able to actually handle messages MaximumMessageLength bytes in
-            // size, we need to allow a bit more than that in FragmentationUtility since this needs
-            // to account for headers and such. 128 bytes is plenty enough for such overhead.
-            m_NetworkParameters.Add(new FragmentationUtility.Parameters() { PayloadCapacity = m_SendQueueBatchSize });
+            // If the user sends a message of exactly m_SendQueueBatchSize length, we'll need an
+            // extra byte to mark it as non-batched and 4 bytes for its length. If the user fills
+            // up the send queue to its capacity (batched messages total m_SendQueueBatchSize), we
+            // still need one extra byte to mark the payload as batched.
+            var fragmentationCapacity = m_SendQueueBatchSize + 1 + 4;
+            m_NetworkParameters.Add(new FragmentationUtility.Parameters() { PayloadCapacity = fragmentationCapacity });
+
             m_NetworkParameters.Add(new BaselibNetworkParameter()
             {
                 maximumPayloadSize = (uint)m_MaximumPacketSize,
@@ -596,24 +599,22 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel
             }
 
             var success = queue.AddEvent(payload);
-            if (!success) // This would be false only when the SendQueue is full already or we are sending a super large message at once
+            if (!success) // No more room in the send queue for the message.
             {
-                // If we are in here data exceeded remaining queue size. This should not happen under normal operation.
-                if (payload.Count > queue.Size)
+                // Flushing the send queue ensures we preserve the order of sends.
+                SendBatchedMessageAndClearQueue(sendTarget, queue);
+                Debug.Assert(queue.IsEmpty() == true);
+                queue.Clear();
+
+                // Try add the message to the queue as there might be enough room now that it's empty.
+                success = queue.AddEvent(payload);
+                if (!success) // Message is too large to fit in the queue. Shouldn't happen under normal operation.
                 {
                     // If data is too large to be batched, flush it out immediately. This happens with large initial spawn packets from Netcode for Gameobjects.
-                    Debug.LogWarning($"Sent {payload.Count} bytes based on delivery method: {networkDelivery}. Event size exceeds sendQueueBatchSize: ({m_SendQueueBatchSize}). This can be the initial payload!");
+                    Debug.LogWarning($"Event of size {payload.Count} too large to fit in send queue (of size {m_SendQueueBatchSize}). Trying to send directly. This could be the initial payload!");
                     Debug.Assert(networkDelivery == NetworkDelivery.ReliableFragmentedSequenced); // Messages like this, should always be sent via the fragmented pipeline.
                     SendMessageInstantly(sendTarget.ClientId, payload, pipeline);
                 }
-                else
-                {
-                    // Since our queue buffer is full then send that right away, clear it and queue this new data
-                    SendBatchedMessageAndClearQueue(sendTarget, queue);
-                    Debug.Assert(queue.IsEmpty() == true);
-                    queue.Clear();
-                    queue.AddEvent(payload);
-                }
             }
         }
 

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

@@ -76,8 +76,6 @@ public IEnumerator PingPong()
             yield return null;
         }
 
-
-
         // Check if can make a simple data exchange (both ways at a time).
         [UnityTest]
         public IEnumerator PingPongSimultaneous()
@@ -113,6 +111,55 @@ public IEnumerator PingPongSimultaneous()
             yield return null;
         }
 
+        [UnityTest]
+        public IEnumerator FilledSendQueueSingleSend()
+        {
+            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_ServerEvents);
+
+            var payload = new ArraySegment<byte>(new byte[UnityTransport.InitialBatchQueueSize]);
+            m_Client1.Send(m_Client1.ServerClientId, payload, NetworkDelivery.ReliableFragmentedSequenced);
+
+            yield return WaitForNetworkEvent(NetworkEvent.Data, m_ServerEvents);
+
+            yield return null;
+        }
+
+        [UnityTest]
+        public IEnumerator FilledSendQueueMultipleSends()
+        {
+            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_ServerEvents);
+
+            var numSends = UnityTransport.InitialBatchQueueSize / 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 - 4]);
+                m_Client1.Send(m_Client1.ServerClientId, payload, NetworkDelivery.ReliableFragmentedSequenced);
+            }
+
+            yield return WaitForNetworkEvent(NetworkEvent.Data, m_ServerEvents);
+
+            // Extra event is for the Connect event.
+            Assert.AreEqual(numSends + 1, m_ServerEvents.Count);
+
+            yield return null;
+        }
+
         // Check making multiple sends to a client in a single frame.
         [UnityTest]
         public IEnumerator MultipleSendsSingleFrame()

com.unity.netcode.adapter.utp/package.json

@@ -6,6 +6,6 @@
   "unity": "2020.3",
   "dependencies": {
     "com.unity.netcode.gameobjects": "1.0.0-pre.2",
-    "com.unity.transport": "1.0.0-pre.6"
+    "com.unity.transport": "1.0.0-pre.7"
   }
 }