fix: Harden error handling

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -25,6 +25,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Ensured all callbacks are wrapped with exception handling to avoid silent errors. (#4161)
 - Issue where a growable `FastBufferWriter` created with a size of 0 never returned from its first write or `TryBeginWrite` call that needed the buffer to grow. (#4224)
 - Issue where a `FastBufferReader` created with an offset and the default length of -1 read past the end of its source. The `ArraySegment<byte>` overload that takes an offset now applies it from the start of the segment. (#4222)
 - Issue where a client promoted to session owner could throw an exception when synchronizing a newly joining client. (#4214)

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

@@ -115,7 +115,16 @@
                 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

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using Unity.Collections;
 using Unity.Entities;
+using UnityEngine;
 
 namespace Unity.Netcode.Components
 {
@@ -69,7 +70,14 @@
 
             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();
@@ -101,7 +109,14 @@
                         // Set the connection in-game
                         commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
                         commandBuffer.AddComponent(entry.Value.Entity, new ConnectionState { NetworkId = entry.Value.NetworkId });
-                        NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
+                        try
+                        {
+                            NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
+                        }
+                        catch (System.Exception ex)
+                        {
+                            Debug.LogException(ex);
+                        }
                         m_TempConnections.Add(entry.Value);
                     }
                 }
@@ -123,8 +138,14 @@
                 foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithAll<ConnectionState>().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);
@@ -139,7 +160,15 @@
             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>
@@ -1747,13 +1768,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

@@ -188,7 +188,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
@@ -321,7 +328,14 @@
                     MessageSize = 0
                 };
                 clientRpcMessage.ReadBuffer = tempBuffer;
-                clientRpcMessage.Handle(ref context);
+                try
+                {
+                    clientRpcMessage.Handle(ref context);
+                }
+                catch (Exception e)
+                {
+                    Debug.LogException(e);
+                }
             }
 
             bufferWriter.Dispose();
@@ -697,8 +711,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;
         }
 
@@ -996,7 +1018,14 @@
             {
                 UpdateNetworkVariableOnOwnershipChanged();
             }
-            OnGainedOwnership();
+            try
+            {
+                OnGainedOwnership();
+            }
+            catch (Exception e)
+            {
+                Debug.LogException(e);
+            }
         }
 
         /// <summary>
@@ -1013,7 +1042,14 @@
 
         internal void InternalOnOwnershipChanged(ulong previous, ulong current)
         {
-            OnOwnershipChanged(previous, current);
+            try
+            {
+                OnOwnershipChanged(previous, current);
+            }
+            catch (Exception e)
+            {
+                Debug.LogException(e);
+            }
         }
 
         /// <summary>