fix: Harden error handling

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -18,6 +18,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Ensured all callbacks are wrapped with exception handling to avoid silent errors. (#4161)
+
 ### Security
 
 ### Obsolete

com.unity.netcode.gameobjects/Documentation~/upgrade-guide.md

@@ -26,7 +26,7 @@ To upgrade an existing project from version 2.x to version 3.x, follow these ste
 4. The API updater should catch any issues and ask if you want to allow it to make changes to your script(s).
 5. If you allow the API updater to make changes for you, then it should auto-update your project's scripts with the correct namespace changes.
 6. If you do not allow the API updater to make changes for you, then the editor will enter safe mode. Open the **Console** window to review the remaining compile errors and resolve the errors. (_[Review the Update Editor assembly definition references section below.](#update-editor-assembly-definition-references)_).
-   
+
 After the API updater finishes and you resolve the compile errors, your project compiles against version 3.x. If the API updater doesn't resolve every reference, refer to [Continue an incomplete API update](#continue-an-incomplete-api-update).
 
 _** If, at any point, you decide to downgrade to the editor version you were using prior to updating to 6.7, then make sure to restore or delete the packages-lock.json file (_assures you are not referencing 6.7 specific packages_), restore your backed up version, and delete your Library folder prior to opening your project with the editor version you were using prior to upgrading to 6.7._

com.unity.netcode.gameobjects/Runtime/Components/Helpers/ComponentController.cs

@@ -396,17 +396,21 @@ protected override void OnNetworkPostSpawn()
         }
 
         /// <inheritdoc/>
-        /// <remarks>
-        /// If overriding this method, it is required that you invoke this base method.
-        /// </remarks>
+        // TODO: Not used anymore
         public override void OnDestroy()
+        {
+            base.OnDestroy();
+        }
+
+
+        internal override void InternalOnDestroy()
         {
             if (m_CoroutineObject.IsRunning)
             {
                 StopCoroutine(m_CoroutineObject.Coroutine);
                 m_CoroutineObject.IsRunning = false;
             }
-            base.OnDestroy();
+            base.InternalOnDestroy();
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Runtime/Components/Helpers/UnifiedBootstrap.cs

@@ -75,7 +75,16 @@ public override bool Initialize(string defaultWorldName)
                 LastCreatedWorld = CreateLocalWorld("LocalWorld");
             }
 
-            OnInitialized?.Invoke();
+            // Always wrap events that can invoke user script in a try-catch to assure any
+            // proceeding script is still executed.
+            try
+            {
+                OnInitialized?.Invoke();
+            }
+            catch (Exception ex)
+            {
+                Debug.LogException(ex);
+            }
 
             return true;
         }

com.unity.netcode.gameobjects/Runtime/Components/Helpers/UnifiedUpdateConnections.cs

@@ -54,7 +54,14 @@ protected override void OnUpdate()
 
                 foreach (var con in m_TempConnections)
                 {
-                    NetworkManager.OnNetCodeDisconnect?.Invoke(con);
+                    try
+                    {
+                        NetworkManager.OnNetCodeDisconnect?.Invoke(con);
+                    }
+                    catch (System.Exception ex)
+                    {
+                        Debug.LogException(ex);
+                    }
                 }
 
                 m_TempConnections.Clear();
@@ -83,7 +90,15 @@ protected override void OnUpdate()
                             // Set the connection in-game
                             commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
                             commandBuffer.AddComponent(entry.Value.Entity, default(ConnectionState));
-                            NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
+
+                            try
+                            {
+                                NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
+                            }
+                            catch (System.Exception ex)
+                            {
+                                Debug.LogException(ex);
+                            }
                             m_TempConnections.Add(entry.Value);
                         }
                     }
@@ -104,8 +119,16 @@ protected override void OnUpdate()
                     foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithEntityAccess())
                     {
                         commandBuffer.RemoveComponent<ConnectionState>(entity);
-                        NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection
-                        { World = World, Entity = entity, NetworkId = networkId.Value });
+
+                        try
+                        {
+                            NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection
+                            { World = World, Entity = entity, NetworkId = networkId.Value });
+                        }
+                        catch (System.Exception ex)
+                        {
+                            Debug.LogException(ex);
+                        }
                     }
                 }
             }
@@ -121,7 +144,15 @@ protected override void OnDestroy()
             foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithEntityAccess())
             {
                 commandBuffer.RemoveComponent<ConnectionState>(entity);
-                NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection { World = World, Entity = entity, NetworkId = networkId.Value });
+
+                try
+                {
+                    NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection { World = World, Entity = entity, NetworkId = networkId.Value });
+                }
+                catch (System.Exception ex)
+                {
+                    Debug.LogException(ex);
+                }
             }
             commandBuffer.Playback(EntityManager);
             base.OnDestroy();

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -711,7 +711,15 @@
             var clientSeverOrHost = LocalClient.IsServer ? LocalClient.IsHost ? "Host" : "Server" : "Client";
             var whenFailed = duringStart ? "start failure" : "failure";
             NetworkLog.LogError($"{clientSeverOrHost} is shutting down due to network transport {whenFailed} of {NetworkManager.NetworkConfig.NetworkTransport.GetType().Name}!");
-            OnTransportFailure?.Invoke();
+
+            try
+            {
+                OnTransportFailure?.Invoke();
+            }
+            catch (Exception ex)
+            {
+                Debug.LogException(ex);
+            }
 
             // If we had a transport failure when trying to start, reset the local client roles and directly invoke the internal shutdown.
             if (duringStart)
@@ -854,12 +862,25 @@
             // Note: ToArray() also allocates. :(
             var response = new NetworkManager.ConnectionApprovalResponse();
             ClientsToApprove[context.SenderId] = response;
-            ConnectionApprovalCallback?.Invoke(
-                new NetworkManager.ConnectionApprovalRequest
-                {
-                    Payload = connectionRequestMessage.ConnectionData,
-                    ClientNetworkId = context.SenderId
-                }, response);
+            try
+            {
+                ConnectionApprovalCallback?.Invoke(
+                    new NetworkManager.ConnectionApprovalRequest
+                    {
+                        Payload = connectionRequestMessage.ConnectionData,
+                        ClientNetworkId = context.SenderId
+                    }, response);
+            }
+            catch (Exception ex)
+            {
+                // A throwing approval handler would otherwise leave a Pending response stranded in
+                // ClientsToApprove, hanging the connecting client until it times out. Deny instead.
+                Debug.LogException(ex);
+                response.Approved = false;
+                response.Pending = false;
+                response.CreatePlayerObject = false;
+                response.Reason = "Connection approval handler threw an exception.";
+            }
         }
 
         /// <summary>
@@ -1748,13 +1769,22 @@
             {
                 //The Transport is set during initialization, thus it is possible for the Transport to be null
                 var transport = NetworkManager.NetworkConfig?.NetworkTransport;
-                if (transport != null)
+                if (transport == null)
+                {
+                    return;
+                }
+                // if the transport throws we need to ensure we finish the shutdown sequence.
+                try
                 {
                     transport.Shutdown();
-                    if (NetworkManager.LogLevel <= LogLevel.Developer)
-                    {
-                        NetworkLog.LogInfo($"{nameof(NetworkConnectionManager)}.{nameof(Shutdown)}() -> {nameof(IsListening)} && {nameof(NetworkManager.NetworkConfig.NetworkTransport)} != null -> {nameof(NetworkTransport)}.{nameof(NetworkTransport.Shutdown)}()");
-                    }
+                }
+                catch (Exception ex)
+                {
+                    Debug.LogException(ex);
+                }
+                if (NetworkManager.LogLevel <= LogLevel.Developer)
+                {
+                    NetworkLog.LogInfo($"{nameof(NetworkConnectionManager)}.{nameof(Shutdown)}() -> {nameof(IsListening)} && {nameof(NetworkManager.NetworkConfig.NetworkTransport)} != null -> {nameof(NetworkTransport)}.{nameof(NetworkTransport.Shutdown)}()");
                 }
             }
         }

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

@@ -137,7 +137,14 @@
                     MessageSize = 0
                 };
                 serverRpcMessage.ReadBuffer = tempBuffer;
-                serverRpcMessage.Handle(ref context);
+                try
+                {
+                    serverRpcMessage.Handle(ref context);
+                }
+                catch (Exception e)
+                {
+                    Debug.LogException(e);
+                }
                 rpcWriteSize = tempBuffer.Length;
             }
             else
@@ -267,7 +274,14 @@
                     MessageSize = 0
                 };
                 clientRpcMessage.ReadBuffer = tempBuffer;
-                clientRpcMessage.Handle(ref context);
+                try
+                {
+                    clientRpcMessage.Handle(ref context);
+                }
+                catch (Exception e)
+                {
+                    Debug.LogException(e);
+                }
             }
 
             bufferWriter.Dispose();
@@ -640,8 +654,16 @@
         /// </remarks>
         internal void SetIsDestroying()
         {
-            // We intentionally invoke this before setting the IsDestroying flag.
-            OnIsDestroying();
+            try
+            {
+                // We intentionally invoke this before setting the IsDestroying flag.
+                OnIsDestroying();
+            }
+            catch (Exception e)
+            {
+                Debug.LogException(e);
+            }
+            // Set outside of the try-catch: a throwing override must not leave this flag false.
             IsDestroying = true;
         }
 
@@ -931,7 +953,14 @@
             {
                 UpdateNetworkVariableOnOwnershipChanged();
             }
-            OnGainedOwnership();
+            try
+            {
+                OnGainedOwnership();
+            }
+            catch (Exception e)
+            {
+                Debug.LogException(e);
+            }
         }
 
         /// <summary>
@@ -948,7 +977,14 @@
 
         internal void InternalOnOwnershipChanged(ulong previous, ulong current)
         {
-            OnOwnershipChanged(previous, current);
+            try
+            {
+                OnOwnershipChanged(previous, current);
+            }
+            catch (Exception e)
+            {
+                Debug.LogException(e);
+            }
         }
 
         /// <summary>