fix: Memory leak in UnityTransport after StartClient failure (#2518)

Author: simon-lemay-unity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed a memory leak in `UnityTransport` that occurred if `StartClient` failed. (#2518)
 - Fixed issue where a client could throw an exception if abruptly disconnected from a network session with one or more spawned `NetworkObject`(s). (#2510)
 - Fixed issue where invalid endpoint addresses were not being detected and returning false from NGO UnityTransport. (#2496)
 - Fixed some errors that could occur if a connection is lost and the loss is detected when attempting to write to the socket. (#2495)

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

@@ -1382,24 +1382,22 @@ public override bool StartServer()
         /// </summary>
         public override void Shutdown()
         {
-            if (!m_Driver.IsCreated)
+            if (m_Driver.IsCreated)
             {
-                return;
-            }
+                // Flush all send queues to the network. NGO can be configured to flush its message
+                // queue on shutdown. But this only calls the Send() method, which doesn't actually
+                // get anything to the network.
+                foreach (var kvp in m_SendQueue)
+                {
+                    SendBatchedMessages(kvp.Key, kvp.Value);
+                }
 
-            // Flush all send queues to the network. NGO can be configured to flush its message
-            // queue on shutdown. But this only calls the Send() method, which doesn't actually
-            // get anything to the network.
-            foreach (var kvp in m_SendQueue)
-            {
-                SendBatchedMessages(kvp.Key, kvp.Value);
+                // The above flush only puts the message in UTP internal buffers, need an update to
+                // actually get the messages on the wire. (Normally a flush send would be sufficient,
+                // but there might be disconnect messages and those require an update call.)
+                m_Driver.ScheduleUpdate().Complete();
             }
 
-            // The above flush only puts the message in UTP internal buffers, need an update to
-            // actually get the messages on the wire. (Normally a flush send would be sufficient,
-            // but there might be disconnect messages and those require an update call.)
-            m_Driver.ScheduleUpdate().Complete();
-
             DisposeInternals();
 
             m_ReliableReceiveQueues.Clear();

com.unity.netcode.gameobjects/Tests/Editor/Transports/UnityTransportTests.cs

@@ -120,7 +120,7 @@ public void UnityTransport_RestartSucceedsAfterFailure()
             Assert.False(transport.StartServer());
 
             LogAssert.Expect(LogType.Error, "Invalid network endpoint: 127.0.0.:4242.");
-            LogAssert.Expect(LogType.Error, $"Network listen address (127.0.0.) is Invalid!");
+            LogAssert.Expect(LogType.Error, "Network listen address (127.0.0.) is Invalid!");
 #if UTP_TRANSPORT_2_0_ABOVE
             LogAssert.Expect(LogType.Error, "Socket creation failed (error Unity.Baselib.LowLevel.Binding+Baselib_ErrorState: Invalid argument (0x01000003) <argument name stripped>");
 #endif
@@ -143,6 +143,22 @@ public void UnityTransport_StartServerWithoutAddresses()
             transport.Shutdown();
         }
 
+        // Check that StartClient returns false with bad connection data.
+        [Test]
+        public void UnityTransport_StartClientFailsWithBadAddress()
+        {
+            UnityTransport transport = new GameObject().AddComponent<UnityTransport>();
+            transport.Initialize();
+
+            transport.SetConnectionData("foobar", 4242);
+            Assert.False(transport.StartClient());
+
+            LogAssert.Expect(LogType.Error, "Invalid network endpoint: foobar:4242.");
+            LogAssert.Expect(LogType.Error, "Target server network address (foobar) is Invalid!");
+
+            transport.Shutdown();
+        }
+
 #if UTP_TRANSPORT_2_0_ABOVE
         [Test]
         public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] string cert, [Values("", null)] string secret)