From a998026ee59cd66f6b0e5f719c985f64ebede31c Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 9 Jan 2024 14:21:33 -0600 Subject: [PATCH 1/5] fix Fixes issue where registering or unregistering named message that is null or empty does not throw an exception. Fixes issue where it was possible that NetworkTransform could try to unregister a named message before it had been assigned a value. --- .../Components/NetworkTransform.cs | 2 +- .../Runtime/Messaging/CustomMessageManager.cs | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Components/NetworkTransform.cs b/com.unity.netcode.gameobjects/Components/NetworkTransform.cs index c463d3955a..7f8f0f7994 100644 --- a/com.unity.netcode.gameobjects/Components/NetworkTransform.cs +++ b/com.unity.netcode.gameobjects/Components/NetworkTransform.cs @@ -2763,7 +2763,7 @@ public override void OnNetworkSpawn() public override void OnNetworkDespawn() { // During destroy, use NetworkBehaviour.NetworkManager as opposed to m_CachedNetworkManager - if (!NetworkManager.ShutdownInProgress && NetworkManager.CustomMessagingManager != null) + if (!NetworkManager.ShutdownInProgress && NetworkManager.CustomMessagingManager != null && !string.IsNullOrEmpty(m_MessageName)) { NetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(m_MessageName); } diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs index 4e15ec6496..6aab172ded 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using Unity.Collections; +using UnityEngine; namespace Unity.Netcode { @@ -199,6 +200,14 @@ internal void InvokeNamedMessage(ulong hash, ulong sender, FastBufferReader read /// The callback to run when a named message is received. public void RegisterNamedMessageHandler(string name, HandleNamedMessageDelegate callback) { + if (string.IsNullOrEmpty(name)) + { + if (m_NetworkManager.LogLevel <= LogLevel.Normal) + { + Debug.LogError($"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!"); + } + return; + } var hash32 = XXHash.Hash32(name); var hash64 = XXHash.Hash64(name); @@ -215,6 +224,15 @@ public void RegisterNamedMessageHandler(string name, HandleNamedMessageDelegate /// The name of the message. public void UnregisterNamedMessageHandler(string name) { + if (string.IsNullOrEmpty(name)) + { + if (m_NetworkManager.LogLevel <= LogLevel.Developer) + { + Debug.LogWarning($"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); + } + return; + } + var hash32 = XXHash.Hash32(name); var hash64 = XXHash.Hash64(name); From 27b21ce550973dfee6b046d009ab4d024e7cd07b Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 9 Jan 2024 14:27:54 -0600 Subject: [PATCH 2/5] update Actually decided to make both cases log an error and raised the minimum loglevel. --- .../Runtime/Messaging/CustomMessageManager.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs index 6aab172ded..1ae806a575 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs @@ -202,7 +202,7 @@ public void RegisterNamedMessageHandler(string name, HandleNamedMessageDelegate { if (string.IsNullOrEmpty(name)) { - if (m_NetworkManager.LogLevel <= LogLevel.Normal) + if (m_NetworkManager.LogLevel <= LogLevel.Error) { Debug.LogError($"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!"); } @@ -226,9 +226,9 @@ public void UnregisterNamedMessageHandler(string name) { if (string.IsNullOrEmpty(name)) { - if (m_NetworkManager.LogLevel <= LogLevel.Developer) + if (m_NetworkManager.LogLevel <= LogLevel.Error) { - Debug.LogWarning($"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); + Debug.LogError($"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); } return; } From 52fab6a73246720f671ecd160d0f90b7842b1e39 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Tue, 9 Jan 2024 14:28:00 -0600 Subject: [PATCH 3/5] test Adding a test to verify that trying to register or unregister a null or empty named message will not throw an exception. --- .../Runtime/Messaging/NamedMessageTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs index 90eeb71cab..46899bdefd 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs @@ -86,6 +86,24 @@ public void NamedMessageIsReceivedOnHostWithContent() Assert.AreEqual(m_ServerNetworkManager.LocalClientId, receivedMessageSender); } + private void MockNamedMessageCallback(ulong sender, FastBufferReader reader) + { + + } + + [Test] + public void NullOrEmptyNamedMessageDoesNotThrowException() + { + LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!"); + m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(string.Empty, MockNamedMessageCallback); + LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!"); + m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(null, MockNamedMessageCallback); + LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); + m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(string.Empty); + LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); + m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(null); + } + [UnityTest] public IEnumerator NamedMessageIsReceivedOnMultipleClientsWithContent() { From 77626fa01cd5a6bd292d56053d3cc99fdef335e2 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Wed, 10 Jan 2024 12:23:08 -0600 Subject: [PATCH 4/5] style removing whitespace --- .../Tests/Runtime/Messaging/NamedMessageTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs index 46899bdefd..ae243cc517 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs @@ -98,7 +98,7 @@ public void NullOrEmptyNamedMessageDoesNotThrowException() m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(string.Empty, MockNamedMessageCallback); LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!"); m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(null, MockNamedMessageCallback); - LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); + LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(string.Empty); LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!"); m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(null); From 1de56629a29bbc22775c7958ee3a3f4520ac8498 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 12 Jan 2024 16:33:40 -0600 Subject: [PATCH 5/5] update Adding change log entries --- com.unity.netcode.gameobjects/CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 775a39a743..8976fb7ff8 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -6,6 +6,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com). +## [Unreleased] + +### Added + +### Fixed + +- Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807) + +### Changed + +- Changed `CustomMessageManager` so it no longer attempts to register or "unregister" a null or empty string and will log an error if this condition occurs. (#2807) + + ## [1.8.0] - 2023-12-12 ### Added