Skip to content

Commit 2717686

Browse files
NoelStephensUnityNoelStephensUnity
authored
fix: client throws a not server exception when destroying a NetworkObject while shutting down [MTT-6210] (#2510)
1 parent 41d071f commit 2717686

File tree

6 files changed

+130
-25
lines changed

6 files changed

+130
-25
lines changed

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

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

1313
### Fixed
1414

15+
- Fixed issue where a client could throw an exception if abruptly disconnected from a network session with one or more spawned `NetworkObject`(s). (#2510)
1516
- Fixed issue where invalid endpoint addresses were not being detected and returning false from NGO UnityTransport. (#2496)
1617
- Fixed some errors that could occur if a connection is lost and the loss is detected when attempting to write to the socket. (#2495)
1718

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

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -531,14 +531,30 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
531531

532532
private void OnDestroy()
533533
{
534-
if (NetworkManager != null && NetworkManager.IsListening && NetworkManager.IsServer == false && IsSpawned &&
534+
// If no NetworkManager is assigned, then just exit early
535+
if (!NetworkManager)
536+
{
537+
return;
538+
}
539+
540+
if (NetworkManager.IsListening && !NetworkManager.IsServer && IsSpawned &&
535541
(IsSceneObject == null || (IsSceneObject.Value != true)))
536542
{
537-
throw new NotServerException($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
543+
// Clients should not despawn NetworkObjects while connected to a session, but we don't want to destroy the current call stack
544+
// if this happens. Instead, we should just generate a network log error and exit early (as long as we are not shutting down).
545+
if (!NetworkManager.ShutdownInProgress)
546+
{
547+
// Since we still have a session connection, log locally and on the server to inform user of this issue.
548+
if (NetworkManager.LogLevel <= LogLevel.Error)
549+
{
550+
NetworkLog.LogErrorServer($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
551+
}
552+
return;
553+
}
554+
// Otherwise, clients can despawn NetworkObjects while shutting down and should not generate any messages when this happens
538555
}
539556

540-
if (NetworkManager != null && NetworkManager.SpawnManager != null &&
541-
NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
557+
if (NetworkManager.SpawnManager != null && NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
542558
{
543559
if (this == networkObject)
544560
{

‎com.unity.netcode.gameobjects/Runtime/Logging/NetworkLog.cs‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -51,35 +51,58 @@ public static class NetworkLog
5151
/// <param name="message">The message to log</param>
5252
public static void LogErrorServer(string message) => LogServer(message, LogType.Error);
5353

54+
internal static NetworkManager NetworkManagerOverride;
55+
5456
private static void LogServer(string message, LogType logType)
5557
{
58+
var networkManager = NetworkManagerOverride ??= NetworkManager.Singleton;
5659
// Get the sender of the local log
57-
ulong localId = NetworkManager.Singleton != null ? NetworkManager.Singleton.LocalClientId : 0;
58-
60+
ulong localId = networkManager?.LocalClientId ?? 0;
61+
bool isServer = networkManager?.IsServer ?? true;
5962
switch (logType)
6063
{
6164
case LogType.Info:
62-
LogInfoServerLocal(message, localId);
65+
if (isServer)
66+
{
67+
LogInfoServerLocal(message, localId);
68+
}
69+
else
70+
{
71+
LogInfo(message);
72+
}
6373
break;
6474
case LogType.Warning:
65-
LogWarningServerLocal(message, localId);
75+
if (isServer)
76+
{
77+
LogWarningServerLocal(message, localId);
78+
}
79+
else
80+
{
81+
LogWarning(message);
82+
}
6683
break;
6784
case LogType.Error:
68-
LogErrorServerLocal(message, localId);
85+
if (isServer)
86+
{
87+
LogErrorServerLocal(message, localId);
88+
}
89+
else
90+
{
91+
LogError(message);
92+
}
6993
break;
7094
}
7195

72-
if (NetworkManager.Singleton != null && !NetworkManager.Singleton.IsServer && NetworkManager.Singleton.NetworkConfig.EnableNetworkLogs)
96+
if (!isServer && networkManager.NetworkConfig.EnableNetworkLogs)
7397
{
74-
7598
var networkMessage = new ServerLogMessage
7699
{
77100
LogType = logType,
78101
Message = message
79102
};
80-
var size = NetworkManager.Singleton.ConnectionManager.SendMessage(ref networkMessage, NetworkDelivery.ReliableFragmentedSequenced, NetworkManager.ServerClientId);
103+
var size = networkManager.ConnectionManager.SendMessage(ref networkMessage, NetworkDelivery.ReliableFragmentedSequenced, NetworkManager.ServerClientId);
81104

82-
NetworkManager.Singleton.NetworkMetrics.TrackServerLogSent(NetworkManager.ServerClientId, (uint)logType, size);
105+
networkManager.NetworkMetrics.TrackServerLogSent(NetworkManager.ServerClientId, (uint)logType, size);
83106
}
84107
}
85108

‎com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeLogAssert.cs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ public void LogWasReceived(LogType type, Regex messageRegex)
147147
if (logEvent.LogType == type && messageRegex.IsMatch(logEvent.Message))
148148
{
149149
found = true;
150+
break;
150151
}
151152
}
152153

@@ -157,6 +158,23 @@ public void LogWasReceived(LogType type, Regex messageRegex)
157158
}
158159
}
159160

161+
public bool HasLogBeenReceived(LogType type, string message)
162+
{
163+
var found = false;
164+
lock (m_Lock)
165+
{
166+
foreach (var logEvent in AllLogs)
167+
{
168+
if (logEvent.LogType == type && message.Equals(logEvent.Message))
169+
{
170+
found = true;
171+
break;
172+
}
173+
}
174+
}
175+
return found;
176+
}
177+
160178
public void Reset()
161179
{
162180
lock (m_Lock)

‎com.unity.netcode.gameobjects/Tests/Runtime/Metrics/ServerLogsMetricTests.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ private int GetWriteSizeForLog(NetworkLog.LogType logType, string logMessage)
3737
[UnityTest]
3838
public IEnumerator TrackServerLogSentMetric()
3939
{
40+
// Set the client NetworkManager to assure the log is sent
41+
NetworkLog.NetworkManagerOverride = Client;
4042
var waitForSentMetric = new WaitForEventMetricValues<ServerLogEvent>(ClientMetrics.Dispatcher, NetworkMetricTypes.ServerLogSent);
4143

4244
var message = Guid.NewGuid().ToString();
@@ -82,6 +84,12 @@ public IEnumerator TrackServerLogReceivedMetric()
8284
var serializedLength = GetWriteSizeForLog(NetworkLog.LogType.Warning, message);
8385
Assert.AreEqual(serializedLength, receivedMetric.BytesCount);
8486
}
87+
88+
protected override IEnumerator OnTearDown()
89+
{
90+
NetworkLog.NetworkManagerOverride = null;
91+
return base.OnTearDown();
92+
}
8593
}
8694
}
8795
#endif

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

Lines changed: 51 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,24 +52,63 @@ public IEnumerator TestNetworkObjectServerDestroy()
5252
Assert.IsTrue(go == null);
5353
}
5454

55+
56+
public enum ClientDestroyObject
57+
{
58+
ShuttingDown,
59+
ActiveSession
60+
}
5561
/// <summary>
56-
/// Tests that a client cannot destroy a spawned networkobject.
62+
/// Validates the expected behavior when the client-side destroys a <see cref="NetworkObject"/>
5763
/// </summary>
58-
/// <returns></returns>
5964
[UnityTest]
60-
public IEnumerator TestNetworkObjectClientDestroy()
65+
public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject clientDestroyObject)
6166
{
62-
// This is the *SERVER VERSION* of the *CLIENT PLAYER*
63-
var serverClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
64-
yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ServerNetworkManager, serverClientPlayerResult);
67+
var isShuttingDown = clientDestroyObject == ClientDestroyObject.ShuttingDown;
68+
var clientPlayer = m_ClientNetworkManagers[0].LocalClient.PlayerObject;
69+
var clientId = clientPlayer.OwnerClientId;
6570

66-
// This is the *CLIENT VERSION* of the *CLIENT PLAYER*
67-
var clientClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
68-
yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ClientNetworkManagers[0], clientClientPlayerResult);
71+
//destroying a NetworkObject while shutting down is allowed
72+
if (isShuttingDown)
73+
{
74+
m_ClientNetworkManagers[0].Shutdown();
75+
}
76+
else
77+
{
78+
LogAssert.ignoreFailingMessages = true;
79+
NetworkLog.NetworkManagerOverride = m_ClientNetworkManagers[0];
80+
}
6981

70-
// destroy the client player, this is not allowed
71-
LogAssert.Expect(LogType.Exception, "NotServerException: Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead.");
72-
Object.DestroyImmediate(clientClientPlayerResult.Result.gameObject);
82+
Object.DestroyImmediate(clientPlayer.gameObject);
83+
84+
// destroying a NetworkObject while a session is active is not allowed
85+
if (!isShuttingDown)
86+
{
87+
yield return WaitForConditionOrTimeOut(HaveLogsBeenReceived);
88+
AssertOnTimeout($"Not all expected logs were received when destroying a {nameof(NetworkObject)} on the client side during an active session!");
89+
}
90+
}
91+
92+
private bool HaveLogsBeenReceived()
93+
{
94+
if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, "[Netcode] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
95+
{
96+
return false;
97+
}
98+
99+
if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, $"[Netcode-Server Sender={m_ClientNetworkManagers[0].LocalClientId}] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
100+
{
101+
return false;
102+
}
103+
104+
return true;
105+
}
106+
107+
protected override IEnumerator OnTearDown()
108+
{
109+
NetworkLog.NetworkManagerOverride = null;
110+
LogAssert.ignoreFailingMessages = false;
111+
return base.OnTearDown();
73112
}
74113
}
75114
}

0 commit comments

Comments
 (0)