From edce75a1186bd29580b7caeafc825c56e1790218 Mon Sep 17 00:00:00 2001 From: 0xm1nam0 Date: Fri, 25 Sep 2026 05:50:54 +0800 Subject: [PATCH] fix: clean up authenticator when server startup fails Remove NetworkManager's authentication listener and stop the authenticator when NetworkServer.Listen throws, preserving external listeners and the original startup exception. Add regression tests for server and host startup failures, retry, and authenticator cleanup exceptions. Validated with Unity 2022.3.62f3: all 18 NetworkManager EditMode tests pass. The same tests against unmodified upstream fail in all three new regression cases. Change-Id: I1febc7820d8fbc65d91a33d0c9e3420ef09bfad6 --- Assets/Mirror/Core/NetworkManager.cs | 23 ++++- .../NetworkManager/NetworkManagerTest.cs | 88 +++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/Assets/Mirror/Core/NetworkManager.cs b/Assets/Mirror/Core/NetworkManager.cs index 8a548b1679e..9264c8c056d 100644 --- a/Assets/Mirror/Core/NetworkManager.cs +++ b/Assets/Mirror/Core/NetworkManager.cs @@ -320,7 +320,28 @@ void SetupServer() ConfigureHeadlessFrameRate(); // start listening to network connections - NetworkServer.Listen(maxConnections); + try + { + NetworkServer.Listen(maxConnections); + } + catch + { + // Listen can throw before active is set. StopServer then returns early, + // so roll back only the authentication listener owned by this manager. + if (authenticator != null) + { + authenticator.OnServerAuthenticated.RemoveListener(OnServerAuthenticated); + try + { + authenticator.OnStopServer(); + } + catch (Exception cleanupError) + { + Debug.LogWarning($"Authenticator startup rollback failed: {cleanupError.GetType().Name}"); + } + } + throw; + } // this must be after Listen(), since that registers the default message handlers RegisterServerMessages(); diff --git a/Assets/Mirror/Tests/Editor/NetworkManager/NetworkManagerTest.cs b/Assets/Mirror/Tests/Editor/NetworkManager/NetworkManagerTest.cs index c9aa027469c..749577ddf8c 100644 --- a/Assets/Mirror/Tests/Editor/NetworkManager/NetworkManagerTest.cs +++ b/Assets/Mirror/Tests/Editor/NetworkManager/NetworkManagerTest.cs @@ -1,6 +1,7 @@ using System; using NUnit.Framework; using UnityEngine; +using UnityEngine.TestTools; namespace Mirror.Tests.NetworkManagers { @@ -29,6 +30,93 @@ public void StartServerTest() Assert.That(NetworkServer.active, Is.True); } + public class FailingStartTransport : MemoryTransport + { + public readonly InvalidOperationException failure = new InvalidOperationException("ServerStart failed"); + + public override void ServerStart() => throw failure; + } + + public class StartupAuthenticator : NetworkAuthenticator + { + public int startCalls; + public int stopCalls; + public bool throwOnStop; + + public override void OnStartServer() => ++startCalls; + + public override void OnStopServer() + { + ++stopCalls; + if (throwOnStop) + { + throw new InvalidOperationException("Authenticator cleanup failed"); + } + } + } + + [TestCase(false)] + [TestCase(true)] + public void FailedListenRemovesOnlyManagerAuthenticationListener(bool host) + { + FailingStartTransport failingTransport = gameObject.AddComponent(); + manager.transport = failingTransport; + Transport.active = failingTransport; + NetworkServer.listen = true; + StartupAuthenticator authenticator = gameObject.AddComponent(); + manager.authenticator = authenticator; + int externalCalls = 0; + authenticator.OnServerAuthenticated.AddListener(_ => ++externalCalls); + + TestDelegate start = host ? (TestDelegate)manager.StartHost : manager.StartServer; + Assert.That(Assert.Throws(start), Is.SameAs(failingTransport.failure)); + Assert.That(NetworkServer.active, Is.False); + + // StopServer returns early while inactive, so startup must undo its listener. + manager.StopServer(); + NetworkConnectionToClient connection = new NetworkConnectionToClient(1); + authenticator.OnServerAuthenticated.Invoke(connection); + Assert.That(connection.isAuthenticated, Is.False); + Assert.That(externalCalls, Is.EqualTo(1)); + Assert.That(authenticator.startCalls, Is.EqualTo(1)); + Assert.That(authenticator.stopCalls, Is.EqualTo(1)); + + // Clean up the failed transport, then retry using the normal transport. + NetworkServer.Shutdown(); + manager.transport = transport; + Transport.active = transport; + manager.StartServer(); + authenticator.OnServerAuthenticated.Invoke(connection); + Assert.That(connection.isAuthenticated, Is.True); + Assert.That(externalCalls, Is.EqualTo(2)); + manager.StopServer(); + connection.isAuthenticated = false; + authenticator.OnServerAuthenticated.Invoke(connection); + Assert.That(connection.isAuthenticated, Is.False); + Assert.That(externalCalls, Is.EqualTo(3)); + Assert.That(authenticator.startCalls, Is.EqualTo(2)); + Assert.That(authenticator.stopCalls, Is.EqualTo(2)); + } + + [Test] + public void FailedListenPreservesOriginalExceptionWhenAuthenticatorCleanupThrows() + { + FailingStartTransport failingTransport = gameObject.AddComponent(); + manager.transport = failingTransport; + Transport.active = failingTransport; + NetworkServer.listen = true; + StartupAuthenticator authenticator = gameObject.AddComponent(); + manager.authenticator = authenticator; + authenticator.throwOnStop = true; + + LogAssert.Expect(LogType.Warning, "Authenticator startup rollback failed: InvalidOperationException"); + Assert.That(Assert.Throws(manager.StartServer), Is.SameAs(failingTransport.failure)); + NetworkConnectionToClient connection = new NetworkConnectionToClient(1); + authenticator.OnServerAuthenticated.Invoke(connection); + Assert.That(connection.isAuthenticated, Is.False); + Assert.That(authenticator.stopCalls, Is.EqualTo(1)); + } + [Test] public void StopServerTest() {