Skip to content

Commit 2d686bf

Browse files
EmandMNoelStephensUnity
EmandM
and
NoelStephensUnity
authored
fix: Harden error handling (#4161)
* fix: Harden error handling * Code review comments * Small updates --------- Co-authored-by: Noel Stephens <noel.stephens@unity3d.com>
1 parent 9207dc0 commit 2d686bf

19 files changed

Lines changed: 596 additions & 170 deletions

‎com.unity.netcode.gameobjects/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
2525

2626
### Fixed
2727

28+
- Ensured all callbacks are wrapped with exception handling to avoid silent errors. (#4161)
2829
- Issue where `NetworkTime` reported a `TickOffset` of almost a whole tick, instead of zero, for some times that land on a tick (for example 0.3 at a tick rate of 10), which put `TickWithPartial` one tick ahead or, in long sessions, `Tick` one tick behind. Times between minus one tick and zero now report tick -1 with a positive `TickOffset`, as other negative times do. (#4228)
2930
- Issue where bits written with `BitWriter` were lost when a later `BitWriter.TryBeginWriteBits` call in the same bitwise context grew the `FastBufferWriter`. The bytes written earlier in that context were replaced with zeros or uninitialized memory. (#4226)
3031
- 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)

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -396,17 +396,21 @@ protected override void OnNetworkPostSpawn()
396396
}
397397

398398
/// <inheritdoc/>
399-
/// <remarks>
400-
/// If overriding this method, it is required that you invoke this base method.
401-
/// </remarks>
399+
// TODO: Not used anymore
402400
public override void OnDestroy()
401+
{
402+
base.OnDestroy();
403+
}
404+
405+
406+
internal override void InternalOnDestroy()
403407
{
404408
if (m_CoroutineObject.IsRunning)
405409
{
406410
StopCoroutine(m_CoroutineObject.Coroutine);
407411
m_CoroutineObject.IsRunning = false;
408412
}
409-
base.OnDestroy();
413+
base.InternalOnDestroy();
410414
}
411415

412416
/// <summary>

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,16 @@ public override bool Initialize(string defaultWorldName)
115115
LastCreatedWorld = CreateLocalWorld("LocalWorld");
116116
}
117117

118-
OnInitialized?.Invoke();
118+
// Always wrap events that can invoke user script in a try-catch to assure any
119+
// proceeding script is still executed.
120+
try
121+
{
122+
OnInitialized?.Invoke();
123+
}
124+
catch (Exception ex)
125+
{
126+
Debug.LogException(ex);
127+
}
119128

120129
return true;
121130
}

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

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Collections.Generic;
33
using Unity.Collections;
44
using Unity.Entities;
5+
using UnityEngine;
56

67
namespace Unity.Netcode.Components
78
{
@@ -69,7 +70,14 @@ protected override void OnUpdate()
6970

7071
foreach (var con in m_TempConnections)
7172
{
72-
NetworkManager.OnNetCodeDisconnect?.Invoke(con);
73+
try
74+
{
75+
NetworkManager.OnNetCodeDisconnect?.Invoke(con);
76+
}
77+
catch (System.Exception ex)
78+
{
79+
Debug.LogException(ex);
80+
}
7381
}
7482

7583
m_TempConnections.Clear();
@@ -101,7 +109,14 @@ protected override void OnUpdate()
101109
// Set the connection in-game
102110
commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
103111
commandBuffer.AddComponent(entry.Value.Entity, new ConnectionState { NetworkId = entry.Value.NetworkId });
104-
NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
112+
try
113+
{
114+
NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
115+
}
116+
catch (System.Exception ex)
117+
{
118+
Debug.LogException(ex);
119+
}
105120
m_TempConnections.Add(entry.Value);
106121
}
107122
}
@@ -123,8 +138,14 @@ protected override void OnUpdate()
123138
foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithAll<ConnectionState>().WithEntityAccess())
124139
{
125140
commandBuffer.RemoveComponent<ConnectionState>(entity);
126-
NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection
127-
{ World = World, Entity = entity, NetworkId = networkId.Value });
141+
try
142+
{
143+
NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection { World = World, Entity = entity, NetworkId = networkId.Value });
144+
}
145+
catch (System.Exception ex)
146+
{
147+
Debug.LogException(ex);
148+
}
128149
}
129150
}
130151
commandBuffer.Playback(EntityManager);
@@ -139,7 +160,15 @@ protected override void OnDestroy()
139160
foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithEntityAccess())
140161
{
141162
commandBuffer.RemoveComponent<ConnectionState>(entity);
142-
NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection { World = World, Entity = entity, NetworkId = networkId.Value });
163+
164+
try
165+
{
166+
NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection { World = World, Entity = entity, NetworkId = networkId.Value });
167+
}
168+
catch (System.Exception ex)
169+
{
170+
Debug.LogException(ex);
171+
}
143172
}
144173
commandBuffer.Playback(EntityManager);
145174
base.OnDestroy();

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

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,15 @@ internal void TransportFailureEventHandler(bool duringStart = false)
711711
var clientSeverOrHost = LocalClient.IsServer ? LocalClient.IsHost ? "Host" : "Server" : "Client";
712712
var whenFailed = duringStart ? "start failure" : "failure";
713713
NetworkLog.LogError($"{clientSeverOrHost} is shutting down due to network transport {whenFailed} of {NetworkManager.NetworkConfig.NetworkTransport.GetType().Name}!");
714-
OnTransportFailure?.Invoke();
714+
715+
try
716+
{
717+
OnTransportFailure?.Invoke();
718+
}
719+
catch (Exception ex)
720+
{
721+
Debug.LogException(ex);
722+
}
715723

716724
// If we had a transport failure when trying to start, reset the local client roles and directly invoke the internal shutdown.
717725
if (duringStart)
@@ -854,12 +862,25 @@ internal void ApproveConnection(ref ConnectionRequestMessage connectionRequestMe
854862
// Note: ToArray() also allocates. :(
855863
var response = new NetworkManager.ConnectionApprovalResponse();
856864
ClientsToApprove[context.SenderId] = response;
857-
ConnectionApprovalCallback?.Invoke(
858-
new NetworkManager.ConnectionApprovalRequest
859-
{
860-
Payload = connectionRequestMessage.ConnectionData,
861-
ClientNetworkId = context.SenderId
862-
}, response);
865+
try
866+
{
867+
ConnectionApprovalCallback?.Invoke(
868+
new NetworkManager.ConnectionApprovalRequest
869+
{
870+
Payload = connectionRequestMessage.ConnectionData,
871+
ClientNetworkId = context.SenderId
872+
}, response);
873+
}
874+
catch (Exception ex)
875+
{
876+
// A throwing approval handler would otherwise leave a Pending response stranded in
877+
// ClientsToApprove, hanging the connecting client until it times out. Deny instead.
878+
Debug.LogException(ex);
879+
response.Approved = false;
880+
response.Pending = false;
881+
response.CreatePlayerObject = false;
882+
response.Reason = "Connection approval handler threw an exception.";
883+
}
863884
}
864885

865886
/// <summary>
@@ -1747,13 +1768,22 @@ internal void Shutdown()
17471768
{
17481769
//The Transport is set during initialization, thus it is possible for the Transport to be null
17491770
var transport = NetworkManager.NetworkConfig?.NetworkTransport;
1750-
if (transport != null)
1771+
if (transport == null)
1772+
{
1773+
return;
1774+
}
1775+
// if the transport throws we need to ensure we finish the shutdown sequence.
1776+
try
17511777
{
17521778
transport.Shutdown();
1753-
if (NetworkManager.LogLevel <= LogLevel.Developer)
1754-
{
1755-
NetworkLog.LogInfo($"{nameof(NetworkConnectionManager)}.{nameof(Shutdown)}() -> {nameof(IsListening)} && {nameof(NetworkManager.NetworkConfig.NetworkTransport)} != null -> {nameof(NetworkTransport)}.{nameof(NetworkTransport.Shutdown)}()");
1756-
}
1779+
}
1780+
catch (Exception ex)
1781+
{
1782+
Debug.LogException(ex);
1783+
}
1784+
if (NetworkManager.LogLevel <= LogLevel.Developer)
1785+
{
1786+
NetworkLog.LogInfo($"{nameof(NetworkConnectionManager)}.{nameof(Shutdown)}() -> {nameof(IsListening)} && {nameof(NetworkManager.NetworkConfig.NetworkTransport)} != null -> {nameof(NetworkTransport)}.{nameof(NetworkTransport.Shutdown)}()");
17571787
}
17581788
}
17591789
}

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

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,14 @@ internal void __endSendServerRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
188188
MessageSize = 0
189189
};
190190
serverRpcMessage.ReadBuffer = tempBuffer;
191-
serverRpcMessage.Handle(ref context);
191+
try
192+
{
193+
serverRpcMessage.Handle(ref context);
194+
}
195+
catch (Exception e)
196+
{
197+
Debug.LogException(e);
198+
}
192199
rpcWriteSize = tempBuffer.Length;
193200
}
194201
else
@@ -321,7 +328,14 @@ internal void __endSendClientRpc(ref FastBufferWriter bufferWriter, uint rpcMeth
321328
MessageSize = 0
322329
};
323330
clientRpcMessage.ReadBuffer = tempBuffer;
324-
clientRpcMessage.Handle(ref context);
331+
try
332+
{
333+
clientRpcMessage.Handle(ref context);
334+
}
335+
catch (Exception e)
336+
{
337+
Debug.LogException(e);
338+
}
325339
}
326340

327341
bufferWriter.Dispose();
@@ -697,8 +711,16 @@ protected internal virtual void OnIsDestroying()
697711
/// </remarks>
698712
internal void SetIsDestroying()
699713
{
700-
// We intentionally invoke this before setting the IsDestroying flag.
701-
OnIsDestroying();
714+
try
715+
{
716+
// We intentionally invoke this before setting the IsDestroying flag.
717+
OnIsDestroying();
718+
}
719+
catch (Exception e)
720+
{
721+
Debug.LogException(e);
722+
}
723+
// Set outside of the try-catch: a throwing override must not leave this flag false.
702724
IsDestroying = true;
703725
}
704726

@@ -996,7 +1018,14 @@ internal void InternalOnGainedOwnership()
9961018
{
9971019
UpdateNetworkVariableOnOwnershipChanged();
9981020
}
999-
OnGainedOwnership();
1021+
try
1022+
{
1023+
OnGainedOwnership();
1024+
}
1025+
catch (Exception e)
1026+
{
1027+
Debug.LogException(e);
1028+
}
10001029
}
10011030

10021031
/// <summary>
@@ -1013,7 +1042,14 @@ protected virtual void OnOwnershipChanged(ulong previous, ulong current)
10131042

10141043
internal void InternalOnOwnershipChanged(ulong previous, ulong current)
10151044
{
1016-
OnOwnershipChanged(previous, current);
1045+
try
1046+
{
1047+
OnOwnershipChanged(previous, current);
1048+
}
1049+
catch (Exception e)
1050+
{
1051+
Debug.LogException(e);
1052+
}
10171053
}
10181054

10191055
/// <summary>

0 commit comments

Comments
 (0)