Skip to content

fix: NetworkManager.ShutdownInProgress not being reset #2661

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

Merged
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Fixed

- Fixed issue where invoking `NetworkManager.Shutdown` within `NetworkManager.OnClientStopped` or `NetworkManager.OnServerStopped` would force `NetworkManager.ShutdownInProgress` to remain true after completing the shutdown process. (#2661)
- Fixed issue with client synchronization of position when using half precision and the delta position reaches the maximum value and is collapsed on the host prior to being forwarded to the non-owner clients. (#2636)
- Fixed issue with scale not synchronizing properly depending upon the spawn order of NetworkObjects. (#2636)
- Fixed issue position was not properly transitioning between ownership changes with an owner authoritative NetworkTransform. (#2636)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,9 @@ internal void ShutdownInternal()
OnServerStopped?.Invoke(ConnectionManager.LocalClient.IsClient);
}

// In the event shutdown is invoked within OnClientStopped or OnServerStopped, set it to false again
m_ShuttingDown = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just move the existing place where it's set to false, instead of setting it twice? Is there a reason it should always be false in OnServerStopped and OnClientStopped?

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone is checking ShutdownInProgress it would return true if it was not set to false...
Yeah... it is a weird thing... so... OnServerStopped and OnClientStopped could invoke other (generic) methods that depend upon that value being false (i.e. validating that the shutdown has completed...which it really hasn't 100% completed...but we are invoking those callbacks at that point in time... so... one of those scenarios... keep it true and someone will complain that it still thinks it is shutting down when it is notifying that it stopped...set it to false...and someone might invoke shutdown again... for--->insert various reasons here<---... 😹 )

The compromise was to just make sure it was no longer shutting down after potentially invoking user code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels like it could use some conceptual rework... but not for 1.6. We can go with this for now...

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep... 💯%


// Reset the client's roles
ConnectionManager.LocalClient.SetRole(false, false);

Expand Down
94 changes: 81 additions & 13 deletions testproject/Assets/Tests/Runtime/NetworkManagerTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections;
using System.Linq;
using NUnit.Framework;
using Unity.Netcode;
using Unity.Netcode.TestHelpers.Runtime;
Expand Down Expand Up @@ -40,12 +41,15 @@ public NetworkManagerTests(UseSceneManagement useSceneManagement)

private void OnClientConnectedCallback(NetworkObject networkObject, int numberOfTimesInvoked, bool isHost, bool isClient, bool isServer)
{
m_NetworkObject = networkObject;
m_NetworkObjectWasSpawned = networkObject.IsSpawned;
m_NetworkBehaviourIsHostWasSet = isHost;
m_NetworkBehaviourIsClientWasSet = isClient;
m_NetworkBehaviourIsServerWasSet = isServer;
m_NumberOfTimesInvoked = numberOfTimesInvoked;
if (networkObject != null)
{
m_NetworkObject = networkObject;
m_NetworkObjectWasSpawned = networkObject.IsSpawned;
m_NetworkBehaviourIsHostWasSet = isHost;
m_NetworkBehaviourIsClientWasSet = isClient;
m_NetworkBehaviourIsServerWasSet = isServer;
m_NumberOfTimesInvoked = numberOfTimesInvoked;
}
}

private bool TestComponentFound()
Expand Down Expand Up @@ -96,22 +100,86 @@ public void ValidateHostSettings()
Assert.IsTrue(m_NetworkBehaviourIsServerWasSet, $"IsServer was not true when OnClientConnectedCallback was invoked!");
}

public enum ShutdownChecks
{
Server,
Client
}

protected override void OnNewClientCreated(NetworkManager networkManager)
{
networkManager.NetworkConfig.EnableSceneManagement = m_EnableSceneManagement;
foreach (var prefab in m_ServerNetworkManager.NetworkConfig.Prefabs.Prefabs)
{
networkManager.NetworkConfig.Prefabs.Add(prefab);
}
base.OnNewClientCreated(networkManager);
}

/// <summary>
/// Validate shutting down a second time does not cause an exception.
/// </summary>
[UnityTest]
public IEnumerator ValidateShutdown()
public IEnumerator ValidateShutdown([Values] ShutdownChecks shutdownCheck)
{
// Register for the server stopped notification so we know we have shutdown completely
m_ServerNetworkManager.OnServerStopped += M_ServerNetworkManager_OnServerStopped;
// Shutdown
m_ServerNetworkManager.Shutdown();


if (shutdownCheck == ShutdownChecks.Server)
{
// Register for the server stopped notification so we know we have shutdown completely
m_ServerNetworkManager.OnServerStopped += OnServerStopped;
// Shutdown
m_ServerNetworkManager.Shutdown();
}
else
{
// For this test (simplify the complexity) with a late joining client, just remove the
// in-scene placed NetworkObject prior to the client connecting
// (We are testing the shutdown sequence)
var spawnedObjects = m_ServerNetworkManager.SpawnManager.SpawnedObjectsList.ToList();

for (int i = spawnedObjects.Count - 1; i >= 0; i--)
{
var spawnedObject = spawnedObjects[i];
if (spawnedObject.IsSceneObject != null && spawnedObject.IsSceneObject.Value)
{
spawnedObject.Despawn();
}
}

yield return s_DefaultWaitForTick;

yield return CreateAndStartNewClient();

// Register for the server stopped notification so we know we have shutdown completely
m_ClientNetworkManagers[0].OnClientStopped += OnClientStopped;
m_ClientNetworkManagers[0].Shutdown();
}

// Let the network manager instance shutdown
yield return s_DefaultWaitForTick;

// Validate the shutdown is no longer in progress
if (shutdownCheck == ShutdownChecks.Server)
{
Assert.False(m_ServerNetworkManager.ShutdownInProgress, $"[{shutdownCheck}] Shutdown in progress was still detected!");
}
else
{
Assert.False(m_ClientNetworkManagers[0].ShutdownInProgress, $"[{shutdownCheck}] Shutdown in progress was still detected!");
}
}

private void OnClientStopped(bool obj)
{
m_ServerNetworkManager.OnServerStopped -= OnClientStopped;
// Verify that we can invoke shutdown again without an exception
m_ServerNetworkManager.Shutdown();
}

private void M_ServerNetworkManager_OnServerStopped(bool obj)
private void OnServerStopped(bool obj)
{
m_ServerNetworkManager.OnServerStopped -= M_ServerNetworkManager_OnServerStopped;
m_ServerNetworkManager.OnServerStopped -= OnServerStopped;
// Verify that we can invoke shutdown again without an exception
m_ServerNetworkManager.Shutdown();
}
Expand Down