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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open

NoelStephensUnity wants to merge 11 commits into develop-2.0.0 from fix/3552-networkmanager-fails-to-cleanup-if-exception-thrown-during-start

com.unity.netcode.gameobjects/CHANGELOG.md

-Original file line number
+Diff line change
@@ Expand Up @@
     ### Fixed
+    - Fixed issue where `NetworkManager` was not cleaning itself up if an exception was thrown while starting. (#3864)
     - 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)
@@ Expand Down @@

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

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -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

-Original file line number
+Diff line change
@@ Expand Up @@
                 {
                     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
                     {
@@ Expand All @@
                 }
                 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)
@@ Expand Down @@

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

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -154,21 +154,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);

    @@ -177,9 +177,9 @@ public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] st
  
                }

                finally

                {

                    if (supportingGO != null)

                    if (supportingGo != null)

                    {

                        Object.DestroyImmediate(supportingGO);

                        Object.DestroyImmediate(supportingGo);

                    }

                }

            }

0 ...cts/Tests/Runtime/ClientApprovalDenied.cs → ...untime/Connection/ClientApprovalDenied.cs

File renamed without changes.

0 ...ests/Runtime/ClientApprovalDenied.cs.meta → ...e/Connection/ClientApprovalDenied.cs.meta

File renamed without changes.

0 ...ests/Runtime/ClientOnlyConnectionTests.cs → ...e/Connection/ClientOnlyConnectionTests.cs

File renamed without changes.

0 ...Runtime/ClientOnlyConnectionTests.cs.meta → ...nection/ClientOnlyConnectionTests.cs.meta

File renamed without changes.

0 ...jects/Tests/Runtime/ConnectionApproval.cs → .../Runtime/Connection/ConnectionApproval.cs

File renamed without changes.

0 .../Tests/Runtime/ConnectionApproval.cs.meta → ...ime/Connection/ConnectionApproval.cs.meta

File renamed without changes.

0 ...Runtime/ConnectionApprovalTimeoutTests.cs → ...nection/ConnectionApprovalTimeoutTests.cs

File renamed without changes.

0 ...me/ConnectionApprovalTimeoutTests.cs.meta → ...on/ConnectionApprovalTimeoutTests.cs.meta

File renamed without changes.

0 ...eobjects/Tests/Runtime/DisconnectTests.cs → ...sts/Runtime/Connection/DisconnectTests.cs

File renamed without changes.

0 ...cts/Tests/Runtime/DisconnectTests.cs.meta → ...untime/Connection/DisconnectTests.cs.meta

File renamed without changes.

0 ...ts/Runtime/InvalidConnectionEventsTest.cs → ...Connection/InvalidConnectionEventsTest.cs

File renamed without changes.

0 ...ntime/InvalidConnectionEventsTest.cs.meta → ...ction/InvalidConnectionEventsTest.cs.meta

File renamed without changes.

com.unity.netcode.gameobjects/Tests/Runtime/NetworkManagerStartExceptionTests.cs

-Original file line number
+Diff line change
@@ -0,0 +1,130 @@
+    using System;
+    using System.Collections;
+    using NUnit.Framework;
+    using Unity.Netcode.TestHelpers.Runtime;
+    using Unity.Netcode.Transports.UTP;
+    using UnityEngine;
+    using UnityEngine.TestTools;
+    namespace Unity.Netcode.RuntimeTests
+    {
+        [TestFixture(StartType.Server)]
+        [TestFixture(StartType.Host)]
+        [TestFixture(StartType.Client)]
+        internal class NetworkManagerStartExceptionTests : NetcodeIntegrationTest
+        {
+            protected override int NumberOfClients => 0;
+            public enum StartType
+            {
+                Server,
+                Host,
+                Client
+            }
+            private StartType m_StartType;
+            public NetworkManagerStartExceptionTests(StartType startType) : base(startType == StartType.Server ? HostOrServer.Server : HostOrServer.Host)
+            {
+                m_StartType = startType;
+            }
+            private const string k_ExceptionText = "This is a test exception";
+            private void ThrowExceptionAction()
+            {
+                throw new Exception(k_ExceptionText);
+            }
+            private SessionConfig SessionConfigExceptionThrower()
+            {
+                throw new Exception(k_ExceptionText);
+            }
+            [UnityTest]
+            public IEnumerator VerifyNetworkManagerHandlesExceptionDuringStart()
+            {
+                var startType = m_StartType;
+                NetworkManager toTest;
+                if (startType == StartType.Client)
+                {
+                    toTest = CreateNewClient();
+                }
+                else
+                {
+                    toTest = GetAuthorityNetworkManager();
+                    yield return StopOneClient(toTest);
+                }
+                var transport = toTest.NetworkConfig.NetworkTransport as UnityTransport;
+                Assert.That(transport, Is.Not.Null, "Transport should not be null");
+                var isListening = true;
+                /*
+                 * Test exception being thrown during NetworkManager.Initialize()
+                 */
+                // It's not possible to throw an exception during server initialization
+                if (startType != StartType.Server)
+                {
+                    // OnSessionConfig is called within Initialize only in DAMode
+                    toTest.NetworkConfig.NetworkTopology = NetworkTopologyTypes.DistributedAuthority;
+                    toTest.OnGetSessionConfig += SessionConfigExceptionThrower;
+                    LogAssert.Expect(LogType.Exception, $"Exception: {k_ExceptionText}");
+                    isListening = startType == StartType.Host ? toTest.StartHost() : toTest.StartClient();
+                    Assert.That(isListening, Is.False, "Should not have started after exception during Initialize()");
+                    Assert.That(transport.GetNetworkDriver().IsCreated, Is.False, "NetworkDriver should not be created.");
+                    toTest.OnGetSessionConfig -= SessionConfigExceptionThrower;
+                    toTest.NetworkConfig.NetworkTopology = NetworkTopologyTypes.ClientServer;
+                }
+                /*
+                 * Test exception being thrown after NetworkTransport.StartClient()
+                 */
+                toTest.OnClientStarted += ThrowExceptionAction;
+                toTest.OnServerStarted += ThrowExceptionAction;
+                LogAssert.Expect(LogType.Exception, $"Exception: {k_ExceptionText}");
+                isListening = startType switch
+                {
+                    StartType.Server => toTest.StartServer(),
+                    StartType.Host => toTest.StartHost(),
+                    StartType.Client => toTest.StartClient(),
+                    _ => true
+                };
+                Assert.That(isListening, Is.False, "Should not have started after exception during startup");
+                Assert.That(transport.GetNetworkDriver().IsCreated, Is.False, "NetworkDriver should not be created.");
+                Assert.False(toTest.IsServer, "IsServer should be false when NetworkManager failed to start");
+                Assert.False(toTest.IsClient, "IsClient should be false when NetworkManager failed to start");
+                toTest.OnClientStarted -= ThrowExceptionAction;
+                toTest.OnServerStarted -= ThrowExceptionAction;
+                if (startType == StartType.Client)
+                {
+                    // Start the client fully to ensure startup still works with no exceptions
+                    yield return StartClient(toTest);
+                    Assert.That(toTest.IsListening, Is.True, "Client failed to start");
+                    Assert.That(transport.GetNetworkDriver().IsCreated, Is.True, "NetworkDriver should be created.");
+                }
+                else
+                {
+                    var isHost = startType == StartType.Host;
+                    NetcodeIntegrationTestHelpers.StartServer(isHost, toTest);
+                    var hostOrServer = isHost ? "Host" : "Server";
+                    Assert.That(toTest.IsListening, Is.True, $"{hostOrServer} failed to start");
+                    Assert.That(transport.GetNetworkDriver().IsCreated, Is.True, "NetworkDriver should be created.");
+                    yield return CreateAndStartNewClient();
+                }
+            }
+        }
+    }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkManagerStartExceptionTests.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDestroyTests.cs

-Original file line number
+Diff line change
@@ Expand Up / @@ -11,7 +11,7 @@ namespace Unity.Netcode.RuntimeTests @@
         /// <summary>
         /// Tests calling destroy on spawned / unspawned <see cref="NetworkObject"/>s. Expected behavior:
         /// - Server or client destroy on unspawned => Object gets destroyed, no exceptions
-        /// - Server destroy spawned => Object gets destroyed and despawned/destroyed on all clients. Server does not run <see cref="NetworkPrefaInstanceHandler.HandleNetworkPrefabDestroy"/>. Client runs it.
+        /// - Server destroy spawned => Object gets destroyed and despawned/destroyed on all clients. Server does not run <see cref="NetworkPrefabInstanceHandler.HandleNetworkPrefabDestroy"/>. Client runs it.
         /// - Client destroy spawned => throw exception.
         /// </summary>
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

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

Are you sure you want to change the base?

Uh oh!

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

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!