fix: UNetTransport.StartServer should return false when failed (MTT-3365, #854) (#1887)

* fix: UNetTransport.StartServer should return false when failed [#854]

UNetTransport.StartServer now checks the result of the underlying UnityEngine.Networking.NetworkTransport.AddHost against the (undocumented) -1 failure code

Co-authored-by: Fatih Mar <[email protected]>

Author: Jesse Olmer

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -18,7 +18,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed `NetworkTransform` generating false positive rotation delta checks when rolling over between 0 and 360 degrees. (#1890)
 - Fixed client throwing an exception if it has messages in the outbound queue when processing the `NetworkEvent.Disconnect` event and is using UTP. (#1884)
 - Fixed issue during client synchronization if 'ValidateSceneBeforeLoading' returned false it would halt the client synchronization process resulting in a client that was approved but not synchronized or fully connected with the server. (#1883)
-
+- Fixed an issue where UNetTransport.StartServer would return success even if the underlying transport failed to start (#854)
 
 ## [1.0.0-pre.7] - 2022-04-06
 

com.unity.netcode.gameobjects/Runtime/Transports/UNET/UNetTransport.cs

@@ -205,8 +205,8 @@ public override bool StartClient()
         public override bool StartServer()
         {
             var topology = new HostTopology(GetConfig(), MaxConnections);
-            UnityEngine.Networking.NetworkTransport.AddHost(topology, ServerListenPort, null);
-            return true;
+            // Undocumented, but AddHost returns -1 in case of any type of failure. See UNET::NetLibraryManager::AddHost
+            return -1 != UnityEngine.Networking.NetworkTransport.AddHost(topology, ServerListenPort, null);
         }
 
         public override void DisconnectRemoteClient(ulong clientId)

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

@@ -0,0 +1,49 @@
+#if UNITY_UNET_PRESENT
+#pragma warning disable 618 // disable is obsolete
+using NUnit.Framework;
+using Unity.Netcode.Transports.UNET;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.EditorTests
+{
+    public class UNetTransportTests
+    {
+        [Test]
+        public void StartServerReturnsFalseOnFailure()
+        {
+            UNetTransport unet1 = null;
+            UNetTransport unet2 = null;
+
+            try
+            {
+                // Arrange
+
+                // We're expecting an error from UNET, but don't care to validate the specific message
+                LogAssert.ignoreFailingMessages = true;
+
+                var go = new GameObject();
+                unet1 = go.AddComponent<UNetTransport>();
+                unet1.ServerListenPort = 1;
+                unet1.Initialize();
+                unet1.StartServer();
+                unet2 = go.AddComponent<UNetTransport>();
+                unet2.ServerListenPort = 1;
+                unet2.Initialize();
+
+                // Act
+                var result = unet2.StartServer();
+
+                // Assert
+                Assert.IsFalse(result, "UNET fails to initialize against port already in use");
+            }
+            finally
+            {
+                unet1?.Shutdown();
+                unet2?.Shutdown();
+            }
+        }
+    }
+}
+#pragma warning restore 618
+#endif

com.unity.netcode.gameobjects/Tests/Editor/Transports/UNetTransportTests.cs.meta

@@ -24,6 +24,11 @@
             "name": "com.unity.multiplayer.tools",
             "expression": "",
             "define": "MULTIPLAYER_TOOLS"
+        },
+        {
+            "name": "Unity",
+            "expression": "(0,2022.2.0a5)",
+            "define": "UNITY_UNET_PRESENT"
         }
     ]
 }