fix: network manager does not clean up if exception is thrown while starting (#3864)

* fix

Fixes the issue where NetworkManager would not clean up if an exception was thrown while it was starting.

* Move all connection tests into the Connection folder

* Catch any exceptions in PrefabHandler.Instantiate

* Add tests for exceptions thrown during startup

* Update CHANGELOG

* Fix code formatting and remove unused code

* Move more connection tests

* Fix tests

* Update CHANGELOG

* Rename test file

---------

Co-authored-by: Emma <emma.mcmillan@unity3d.com>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -29,8 +29,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
-- Prevented a `NullReferenceException` in `UnityTransport` when using a custom `INetworkStreamDriverConstructor` that doesn't use all the default pipelines and the multiplayer tools package is installed. (#3853)
+- Fixed issue where `NetworkManager` was not cleaning itself up if an exception was thrown while starting. (#3864)
 - Duplicate transport connection events for the same connection will now do nothing. (#3863)
+- Prevented a `NullReferenceException` in `UnityTransport` when using a custom `INetworkStreamDriverConstructor` that doesn't use all the default pipelines and the multiplayer tools package is installed. (#3853)
 - Fixed memory leak in `NetworkAnimator` on clients where `RpcTarget` groups were not being properly disposed due to incorrect type casting of `ProxyRpcTargetGroup` to `RpcTargetGroup`.
 - Fixed issue when using a client-server topology where a `NetworkList` with owner write permissions was resetting sent time and dirty flags after having been spawned on owning clients that were not the spawn authority. (#3850)
 - Fixed an integer overflow that occurred when configuring a large disconnect timeout with Unity Transport. (#3810)

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

@@ -1323,7 +1323,17 @@ public bool StartServer()
             }
             ConnectionManager.LocalClient.ClientId = ServerClientId;
 
-            Initialize(true);
+            try
+            {
+                Initialize(true);
+            }
+            catch (Exception ex)
+            {
+                Debug.LogException(ex);
+                // Always shutdown to assure everything is cleaned up
+                ShutdownInternal();
+                return false;
+            }
 
             try
             {
@@ -1342,11 +1352,12 @@ public bool StartServer()
 
                 ConnectionManager.TransportFailureEventHandler(true);
             }
-            catch (Exception)
+            catch (Exception ex)
             {
-                ConnectionManager.LocalClient.SetRole(false, false);
+                Debug.LogException(ex);
+                // Always shutdown to assure everything is cleaned up
+                ShutdownInternal();
                 IsListening = false;
-                throw;
             }
 
             return IsListening;
@@ -1373,7 +1384,16 @@ public bool StartClient()
                 return false;
             }
 
-            Initialize(false);
+            try
+            {
+                Initialize(false);
+            }
+            catch (Exception ex)
+            {
+                Debug.LogException(ex);
+                ShutdownInternal();
+                return false;
+            }
 
             try
             {
@@ -1391,7 +1411,7 @@ public bool StartClient()
             catch (Exception ex)
             {
                 Debug.LogException(ex);
-                ConnectionManager.LocalClient.SetRole(false, false);
+                ShutdownInternal();
                 IsListening = false;
             }
 
@@ -1419,7 +1439,18 @@ public bool StartHost()
                 return false;
             }
 
-            Initialize(true);
+            try
+            {
+                Initialize(true);
+            }
+            catch (Exception ex)
+            {
+                Debug.LogException(ex);
+                // Always shutdown to assure everything is cleaned up
+                ShutdownInternal();
+                return false;
+            }
+
             try
             {
                 IsListening = NetworkConfig.NetworkTransport.StartServer();
@@ -1437,7 +1468,8 @@ public bool StartHost()
             catch (Exception ex)
             {
                 Debug.LogException(ex);
-                ConnectionManager.LocalClient.SetRole(false, false);
+                // Always shutdown to assure everything is cleaned up
+                ShutdownInternal();
                 IsListening = false;
             }
 

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkPrefabHandler.cs

@@ -287,7 +287,15 @@ internal NetworkObject HandleNetworkPrefabSpawn(uint networkPrefabAssetHash, ulo
             {
                 if (m_PrefabAssetToPrefabHandlerWithData.TryGetValue(networkPrefabAssetHash, out var prefabInstanceHandler))
                 {
-                    networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation, instantiationData);
+                    try
+                    {
+                        networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation, instantiationData);
+                    }
+                    catch (Exception ex)
+                    {
+                        Debug.LogException(ex);
+                        return null;
+                    }
                 }
                 else
                 {
@@ -297,7 +305,15 @@ internal NetworkObject HandleNetworkPrefabSpawn(uint networkPrefabAssetHash, ulo
             }
             else if (m_PrefabAssetToPrefabHandler.TryGetValue(networkPrefabAssetHash, out var prefabInstanceHandler))
             {
-                networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation);
+                try
+                {
+                    networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation);
+                }
+                catch (Exception ex)
+                {
+                    Debug.LogException(ex);
+                    return null;
+                }
             }
 
             // Now we must make sure this alternate PrefabAsset spawned in place of the prefab asset with the networkPrefabAssetHash (GlobalObjectIdHash)

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

@@ -155,21 +155,21 @@ public void UnityTransport_StartServerWithoutAddresses()
         [Test]
         public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] string cert, [Values("", null)] string secret)
         {
-            var supportingGO = new GameObject();
+            var supportingGo = new GameObject();
             try
             {
-                var networkManager = supportingGO.AddComponent<NetworkManager>(); // NM is required for UTP to work with certificates.
+                var networkManager = supportingGo.AddComponent<NetworkManager>(); // NM is required for UTP to work with certificates.
                 networkManager.NetworkConfig = new NetworkConfig();
-                UnityTransport transport = supportingGO.AddComponent<UnityTransport>();
+                UnityTransport transport = supportingGo.AddComponent<UnityTransport>();
                 networkManager.NetworkConfig.NetworkTransport = transport;
-                transport.Initialize();
+                transport.Initialize(networkManager);
                 transport.SetServerSecrets(serverCertificate: cert, serverPrivateKey: secret);
 
                 // Use encryption, but don't set certificate and check for exception
                 transport.UseEncryption = true;
                 Assert.Throws<System.Exception>(() =>
                 {
-                    networkManager.StartServer();
+                    transport.StartServer();
                 });
                 // Make sure StartServer failed
                 Assert.False(transport.GetNetworkDriver().IsCreated);
@@ -178,9 +178,9 @@ public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] st
             }
             finally
             {
-                if (supportingGO != null)
+                if (supportingGo != null)
                 {
-                    Object.DestroyImmediate(supportingGO);
+                    Object.DestroyImmediate(supportingGo);
                 }
             }
         }