Skip to content

fix: host not generating its own local client disconnect notification #2822

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 11 commits into from
Mar 12, 2024
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 the host was not invoking `OnClientDisconnectCallback` for its own local client when internally shutting down. (#2822)
- Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807)
- Fixed issue where in-scene placed `NetworkObject`s with complex nested children `NetworkObject`s (more than one child in depth) would not synchronize properly if WorldPositionStays was set to true. (#2796)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,12 @@ internal void ShutdownInternal()
IsListening = false;
m_ShuttingDown = false;

// Generate a local notification that the host client is disconnected
if (IsHost)
{
ConnectionManager.InvokeOnClientDisconnectCallback(LocalClientId);
}

if (ConnectionManager.LocalClient.IsClient)
{
// If we were a client, we want to know if we were a host
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,10 @@ protected void CreateAndStartNewClientWithTimeTravel()
protected IEnumerator StopOneClient(NetworkManager networkManager, bool destroy = false)
{
NetcodeIntegrationTestHelpers.StopOneClient(networkManager, destroy);
AddRemoveNetworkManager(networkManager, false);
if (destroy)
{
AddRemoveNetworkManager(networkManager, false);
}
yield return WaitForConditionOrTimeOut(() => !networkManager.IsConnectedClient);
}

Expand Down
51 changes: 51 additions & 0 deletions com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;
using Unity.Netcode.TestHelpers.Runtime;
Expand Down Expand Up @@ -37,7 +38,10 @@ public enum ClientDisconnectType
protected override int NumberOfClients => 1;

private OwnerPersistence m_OwnerPersistence;
private ClientDisconnectType m_ClientDisconnectType;
private bool m_ClientDisconnected;
private Dictionary<NetworkManager, ConnectionEventData> m_DisconnectedEvent = new Dictionary<NetworkManager, ConnectionEventData>();
private ulong m_DisconnectEventClientId;
private ulong m_TransportClientId;
private ulong m_ClientId;

Expand Down Expand Up @@ -89,6 +93,16 @@ private void OnClientDisconnectCallback(ulong obj)
m_ClientDisconnected = true;
}

private void OnConnectionEvent(NetworkManager networkManager, ConnectionEventData connectionEventData)
{
if (connectionEventData.EventType != ConnectionEvent.ClientDisconnected)
{
return;
}

m_DisconnectedEvent.Add(networkManager, connectionEventData);
}

/// <summary>
/// Conditional check to assure the transport to client (and vice versa) mappings are cleaned up
/// </summary>
Expand Down Expand Up @@ -126,6 +140,7 @@ private bool DoesServerStillHaveSpawnedPlayerObject()
public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType clientDisconnectType)
{
m_ClientId = m_ClientNetworkManagers[0].LocalClientId;
m_ClientDisconnectType = clientDisconnectType;

var serverSideClientPlayer = m_ServerNetworkManager.ConnectionManager.ConnectedClients[m_ClientId].PlayerObject;

Expand All @@ -134,18 +149,39 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client
if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient)
{
m_ClientNetworkManagers[0].OnClientDisconnectCallback += OnClientDisconnectCallback;
m_ClientNetworkManagers[0].OnConnectionEvent += OnConnectionEvent;
m_ServerNetworkManager.OnConnectionEvent += OnConnectionEvent;
m_ServerNetworkManager.DisconnectClient(m_ClientId);
}
else
{
m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback;
m_ServerNetworkManager.OnConnectionEvent += OnConnectionEvent;
m_ClientNetworkManagers[0].OnConnectionEvent += OnConnectionEvent;

yield return StopOneClient(m_ClientNetworkManagers[0]);
}

yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected);
AssertOnTimeout("Timed out waiting for client to disconnect!");

if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient)
{
Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ClientNetworkManagers[0]), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent[m_ClientNetworkManagers[0]].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the client {nameof(NetworkManager)} disconnect event entry!");
// Unregister for this event otherwise it will be invoked during teardown
m_ServerNetworkManager.OnConnectionEvent -= OnConnectionEvent;
}
else
{
Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ClientNetworkManagers[0]), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent[m_ClientNetworkManagers[0]].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the client {nameof(NetworkManager)} disconnect event entry!");
}

if (m_OwnerPersistence == OwnerPersistence.DestroyWithOwner)
{
// When we are destroying with the owner, validate the player object is destroyed on the server side
Expand All @@ -161,6 +197,21 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client

yield return WaitForConditionOrTimeOut(TransportIdCleanedUp);
AssertOnTimeout("Timed out waiting for transport and client id mappings to be cleaned up!");

// Validate the host-client generates a OnClientDisconnected event when it shutsdown.
// Only test when the test run is the client disconnecting from the server (otherwise the server will be shutdown already)
if (clientDisconnectType == ClientDisconnectType.ClientDisconnectsFromServer)
{
m_DisconnectedEvent.Clear();
m_ClientDisconnected = false;
m_ServerNetworkManager.Shutdown();

yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected);
AssertOnTimeout("Timed out waiting for host-client to generate disconnect message!");

Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!");
Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == NetworkManager.ServerClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!");
}
}
}
}
47 changes: 23 additions & 24 deletions testproject/Assets/Tests/Runtime/SenderIdTests.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using NUnit.Framework;
using Unity.Collections;
using Unity.Netcode;
Expand Down Expand Up @@ -89,36 +90,34 @@ public IEnumerator WhenSendingMessageFromClientToServer_SenderIdIsCorrect()
Assert.IsTrue(secondReceived);
}


private List<ulong> m_ClientsDisconnected = new List<ulong>();
private ulong m_ClientToValidateDisconnected;

private bool ValidateClientId()
{
return m_ClientsDisconnected.Contains(m_ClientToValidateDisconnected);
}

private void OnClientDisconnected(ulong clientId)
{
m_ClientsDisconnected.Add(clientId);
}

[UnityTest]
public IEnumerator WhenClientDisconnectsFromServer_ClientIdIsCorrect()
{
var firstClientId = FirstClient.LocalClientId;
bool received = false;
void firstCallback(ulong id)
{
Assert.AreEqual(firstClientId, id);
received = true;
}
m_ServerNetworkManager.OnClientDisconnectCallback += firstCallback;
m_ClientsDisconnected.Clear();
m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnected;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a check for this behavior in OnConnectionEvent? InvokeOnClientDisconnectCallback should invoke both of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also discovered an issue with StopOneClient while adding the test... so it was a 2 4 1.
Validated the ConnectionEvent.ClientDisconnected triggers under both scenarios and fixed the integration test method.
👍

m_ClientToValidateDisconnected = m_ClientNetworkManagers[0].LocalClientId;
FirstClient.Shutdown();
yield return WaitForConditionOrTimeOut(ValidateClientId);
AssertOnTimeout($"Timed out waiting for the server to receive Client-{m_ClientToValidateDisconnected} disconnect event!");

yield return new WaitForSeconds(0.2f);

Assert.IsTrue(received);
var secondClientId = SecondClient.LocalClientId;
received = false;

m_ServerNetworkManager.OnClientDisconnectCallback -= firstCallback;
m_ServerNetworkManager.OnClientDisconnectCallback += id =>
{
Assert.AreEqual(secondClientId, id);
received = true;
};
m_ClientToValidateDisconnected = m_ClientNetworkManagers[1].LocalClientId;
SecondClient.Shutdown();

yield return new WaitForSeconds(0.2f);

Assert.IsTrue(received);
yield return WaitForConditionOrTimeOut(ValidateClientId);
AssertOnTimeout($"Timed out waiting for the server to receive Client-{m_ClientToValidateDisconnected} disconnect event!");
}
}
}