Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix: network manager does not clean up if exception is thrown while starting #3864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NoelStephensUnity wants to merge 11 commits into develop-2.0.0
base: develop-2.0.0
Choose a base branch
Loading
from fix/3552-networkmanager-fails-to-cleanup-if-exception-thrown-during-start
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Fixed

- Fixed issue where `NetworkManager` was not cleaning itself up if an exception was thrown while starting. (#3864)
- Fixed memory leak in `NetworkAnimator` on clients where `RpcTarget` groups were not being properly disposed due to incorrect type casting of `ProxyRpcTargetGroup` to `RpcTargetGroup`.
- Fixed issue when using a client-server topology where a `NetworkList` with owner write permissions was resetting sent time and dirty flags after having been spawned on owning clients that were not the spawn authority. (#3850)
- Fixed an integer overflow that occurred when configuring a large disconnect timeout with Unity Transport. (#3810)
Expand Down
48 changes: 40 additions & 8 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,7 +1323,17 @@ public bool StartServer()
}
ConnectionManager.LocalClient.ClientId = ServerClientId;

Initialize(true);
try
{
Initialize(true);
}
catch (Exception ex)
{
Debug.LogException(ex);
// Always shutdown to assure everything is cleaned up
ShutdownInternal();
return false;
}

try
{
Expand All @@ -1342,11 +1352,12 @@ public bool StartServer()

ConnectionManager.TransportFailureEventHandler(true);
}
catch (Exception)
catch (Exception ex)
{
ConnectionManager.LocalClient.SetRole(false, false);
Debug.LogException(ex);
// Always shutdown to assure everything is cleaned up
ShutdownInternal();
IsListening = false;
throw;
}

return IsListening;
Expand All @@ -1373,7 +1384,16 @@ public bool StartClient()
return false;
}

Initialize(false);
try
{
Initialize(false);
}
catch (Exception ex)
{
Debug.LogException(ex);
ShutdownInternal();
return false;
}

try
{
Expand All @@ -1391,7 +1411,7 @@ public bool StartClient()
catch (Exception ex)
{
Debug.LogException(ex);
ConnectionManager.LocalClient.SetRole(false, false);
ShutdownInternal();
IsListening = false;
}

Expand Down Expand Up @@ -1419,7 +1439,18 @@ public bool StartHost()
return false;
}

Initialize(true);
try
{
Initialize(true);
}
catch (Exception ex)
{
Debug.LogException(ex);
// Always shutdown to assure everything is cleaned up
ShutdownInternal();
return false;
}

try
{
IsListening = NetworkConfig.NetworkTransport.StartServer();
Expand All @@ -1437,7 +1468,8 @@ public bool StartHost()
catch (Exception ex)
{
Debug.LogException(ex);
ConnectionManager.LocalClient.SetRole(false, false);
// Always shutdown to assure everything is cleaned up
ShutdownInternal();
IsListening = false;
}

Expand Down
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,15 @@ internal NetworkObject HandleNetworkPrefabSpawn(uint networkPrefabAssetHash, ulo
{
if (m_PrefabAssetToPrefabHandlerWithData.TryGetValue(networkPrefabAssetHash, out var prefabInstanceHandler))
{
networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation, instantiationData);
try
{
networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation, instantiationData);
}
catch (Exception ex)
{
Debug.LogException(ex);
return null;
}
}
else
{
Expand All @@ -297,7 +305,15 @@ internal NetworkObject HandleNetworkPrefabSpawn(uint networkPrefabAssetHash, ulo
}
else if (m_PrefabAssetToPrefabHandler.TryGetValue(networkPrefabAssetHash, out var prefabInstanceHandler))
{
networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation);
try
{
networkObjectInstance = prefabInstanceHandler.Instantiate(ownerClientId, position, rotation);
}
catch (Exception ex)
{
Debug.LogException(ex);
return null;
}
}

// Now we must make sure this alternate PrefabAsset spawned in place of the prefab asset with the networkPrefabAssetHash (GlobalObjectIdHash)
Expand Down
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,21 +154,21 @@ public void UnityTransport_StartServerWithoutAddresses()
[Test]
public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] string cert, [Values("", null)] string secret)
{
var supportingGO = new GameObject();
var supportingGo = new GameObject();
try
{
var networkManager = supportingGO.AddComponent<NetworkManager>(); // NM is required for UTP to work with certificates.
var networkManager = supportingGo.AddComponent<NetworkManager>(); // NM is required for UTP to work with certificates.
networkManager.NetworkConfig = new NetworkConfig();
UnityTransport transport = supportingGO.AddComponent<UnityTransport>();
UnityTransport transport = supportingGo.AddComponent<UnityTransport>();
networkManager.NetworkConfig.NetworkTransport = transport;
transport.Initialize();
transport.Initialize(networkManager);
transport.SetServerSecrets(serverCertificate: cert, serverPrivateKey: secret);

// Use encryption, but don't set certificate and check for exception
transport.UseEncryption = true;
Assert.Throws<System.Exception>(() =>
{
networkManager.StartServer();
transport.StartServer();
});
// Make sure StartServer failed
Assert.False(transport.GetNetworkDriver().IsCreated);
Expand All @@ -177,9 +177,9 @@ public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] st
}
finally
{
if (supportingGO != null)
if (supportingGo != null)
{
Object.DestroyImmediate(supportingGO);
Object.DestroyImmediate(supportingGo);
}
}
}
Expand Down
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System;
using System.Collections;
using NUnit.Framework;
using Unity.Netcode.TestHelpers.Runtime;
using Unity.Netcode.Transports.UTP;
using UnityEngine;
using UnityEngine.TestTools;

namespace Unity.Netcode.RuntimeTests
{
[TestFixture(StartType.Server)]
[TestFixture(StartType.Host)]
[TestFixture(StartType.Client)]
internal class NetworkManagerStartExceptionTests : NetcodeIntegrationTest
{
protected override int NumberOfClients => 0;

public enum StartType
{
Server,
Host,
Client
}

private StartType m_StartType;
public NetworkManagerStartExceptionTests(StartType startType) : base(startType == StartType.Server ? HostOrServer.Server : HostOrServer.Host)
{
m_StartType = startType;
}

private const string k_ExceptionText = "This is a test exception";

private void ThrowExceptionAction()
{
throw new Exception(k_ExceptionText);
}

private SessionConfig SessionConfigExceptionThrower()
{
throw new Exception(k_ExceptionText);
}

[UnityTest]
public IEnumerator VerifyNetworkManagerHandlesExceptionDuringStart()
{
var startType = m_StartType;
NetworkManager toTest;
if (startType == StartType.Client)
{
toTest = CreateNewClient();
}
else
{
toTest = GetAuthorityNetworkManager();
yield return StopOneClient(toTest);
}

var transport = toTest.NetworkConfig.NetworkTransport as UnityTransport;
Assert.That(transport, Is.Not.Null, "Transport should not be null");

var isListening = true;

/*
* Test exception being thrown during NetworkManager.Initialize()
*/

// It's not possible to throw an exception during server initialization
if (startType != StartType.Server)
{
// OnSessionConfig is called within Initialize only in DAMode
toTest.NetworkConfig.NetworkTopology = NetworkTopologyTypes.DistributedAuthority;

toTest.OnGetSessionConfig += SessionConfigExceptionThrower;

LogAssert.Expect(LogType.Exception, $"Exception: {k_ExceptionText}");
isListening = startType == StartType.Host ? toTest.StartHost() : toTest.StartClient();

Assert.That(isListening, Is.False, "Should not have started after exception during Initialize()");
Assert.That(transport.GetNetworkDriver().IsCreated, Is.False, "NetworkDriver should not be created.");

toTest.OnGetSessionConfig -= SessionConfigExceptionThrower;
toTest.NetworkConfig.NetworkTopology = NetworkTopologyTypes.ClientServer;
}

/*
* Test exception being thrown after NetworkTransport.StartClient()
*/

toTest.OnClientStarted += ThrowExceptionAction;
toTest.OnServerStarted += ThrowExceptionAction;

LogAssert.Expect(LogType.Exception, $"Exception: {k_ExceptionText}");
isListening = startType switch
{
StartType.Server => toTest.StartServer(),
StartType.Host => toTest.StartHost(),
StartType.Client => toTest.StartClient(),
_ => true
};

Assert.That(isListening, Is.False, "Should not have started after exception during startup");
Assert.That(transport.GetNetworkDriver().IsCreated, Is.False, "NetworkDriver should not be created.");
Assert.False(toTest.IsServer, "IsServer should be false when NetworkManager failed to start");
Assert.False(toTest.IsClient, "IsClient should be false when NetworkManager failed to start");

toTest.OnClientStarted -= ThrowExceptionAction;
toTest.OnServerStarted -= ThrowExceptionAction;

if (startType == StartType.Client)
{
// Start the client fully to ensure startup still works with no exceptions
yield return StartClient(toTest);

Assert.That(toTest.IsListening, Is.True, "Client failed to start");
Assert.That(transport.GetNetworkDriver().IsCreated, Is.True, "NetworkDriver should be created.");
}
else
{
var isHost = startType == StartType.Host;
NetcodeIntegrationTestHelpers.StartServer(isHost, toTest);
var hostOrServer = isHost ? "Host" : "Server";
Assert.That(toTest.IsListening, Is.True, $"{hostOrServer} failed to start");
Assert.That(transport.GetNetworkDriver().IsCreated, Is.True, "NetworkDriver should be created.");

yield return CreateAndStartNewClient();
}
}

}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Unity.Netcode.RuntimeTests
/// <summary>
/// Tests calling destroy on spawned / unspawned <see cref="NetworkObject"/>s. Expected behavior:
/// - Server or client destroy on unspawned => Object gets destroyed, no exceptions
/// - Server destroy spawned => Object gets destroyed and despawned/destroyed on all clients. Server does not run <see cref="NetworkPrefaInstanceHandler.HandleNetworkPrefabDestroy"/>. Client runs it.
/// - Server destroy spawned => Object gets destroyed and despawned/destroyed on all clients. Server does not run <see cref="NetworkPrefabInstanceHandler.HandleNetworkPrefabDestroy"/>. Client runs it.
/// - Client destroy spawned => throw exception.
/// </summary>

Expand Down
Loading

AltStyle によって変換されたページ (->オリジナル) /