Skip to content

fix: Fix handling of disconnection events in the adapter #1673

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.

Sign up for GitHub

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 6 commits into from
Feb 10, 2022
Merged

fix: Fix handling of disconnection events in the adapter #1673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
93c92a4
Bring over some changes from PR #1649
simon-lemay-unity Feb 10, 2022
fc48cd0
Add test for server disconnect after client disconnect
simon-lemay-unity Feb 10, 2022
45f8d1c
Improve stability of connection tests
simon-lemay-unity Feb 10, 2022
dbf7bbe
Fix handling of Disconnect events
simon-lemay-unity Feb 10, 2022
1783ae9
Add CHANGELOG entry
simon-lemay-unity Feb 10, 2022
77eb9ad
Add PR number to CHANGELOG entry
simon-lemay-unity Feb 10, 2022
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.adapter.utp/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ All notable changes to this package will be documented in this file. The format

- Fixed issue where the server `NetworkEndPoint` would fail to be created when 'Server Listen Address' is empty. (#1636)
- Fixed issue with native collections not all being disposed of when destroying the component without shutting it down properly. This would result in errors in the console and memory leaks. (#1640)
- Fixed and issue where a server would fail to disconnect a client if another client had previously disconnected itself. (#1673)

## [1.0.0-pre.5] - 2022-01-26

Expand Down
78 changes: 44 additions & 34 deletions com.unity.netcode.adapter.utp/Runtime/UnityTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,6 @@ public static implicit operator ConnectionAddressData(NetworkEndPoint d) =>
private State m_State = State.Disconnected;
private NetworkDriver m_Driver;
private NetworkSettings m_NetworkSettings;
private byte[] m_MessageBuffer;
private NetworkConnection m_ServerConnection;
private ulong m_ServerClientId;

private NetworkPipeline m_UnreliableFragmentedPipeline;
Expand Down Expand Up @@ -309,8 +307,8 @@ private bool ClientBindAndConnect()
return false;
}

m_ServerConnection = m_Driver.Connect(serverEndpoint);
m_ServerClientId = ParseClientId(m_ServerConnection);
var serverConnection = m_Driver.Connect(serverEndpoint);
m_ServerClientId = ParseClientId(serverConnection);

return true;
}
Expand Down Expand Up @@ -596,13 +594,14 @@ private void ReceiveMessages(ulong clientId, NetworkPipeline pipeline, DataStrea
private bool ProcessEvent()
{
var eventType = m_Driver.PopEvent(out var networkConnection, out var reader, out var pipeline);
var clientId = ParseClientId(networkConnection);

switch (eventType)
{
case TransportNetworkEvent.Type.Connect:
{
InvokeOnTransportEvent(NetcodeNetworkEvent.Connect,
ParseClientId(networkConnection),
clientId,
default(ArraySegment<byte>),
Time.realtimeSinceStartup);

Expand All @@ -611,30 +610,34 @@ private bool ProcessEvent()
}
case TransportNetworkEvent.Type.Disconnect:
{
if (m_ServerConnection.IsCreated)
// Handle cases where we're a client receiving a Disconnect event. The
// meaning of the event depends on our current state. If we were connected
// then it means we got disconnected. If we were disconnected means that our
// connection attempt has failed.
if (m_State == State.Connected)
{
m_ServerConnection = default;

var reason = reader.ReadByte();
if (reason == (byte)Networking.Transport.Error.DisconnectReason.MaxConnectionAttempts)
{
Debug.LogError("Client failed to connect to server");
}
m_State = State.Disconnected;
m_ServerClientId = default;
}
else if (m_State == State.Disconnected)
{
Debug.LogError("Failed to connect to server.");
m_ServerClientId = default;
}

m_ReliableReceiveQueues.Remove(ParseClientId(networkConnection));
m_ReliableReceiveQueues.Remove(clientId);
ClearSendQueuesForClientId(clientId);

InvokeOnTransportEvent(NetcodeNetworkEvent.Disconnect,
ParseClientId(networkConnection),
clientId,
default(ArraySegment<byte>),
Time.realtimeSinceStartup);

m_State = State.Disconnected;
return true;
}
case TransportNetworkEvent.Type.Data:
{
ReceiveMessages(ParseClientId(networkConnection), pipeline, reader);
ReceiveMessages(clientId, pipeline, reader);
return true;
}
}
Expand Down Expand Up @@ -680,15 +683,36 @@ private static unsafe NetworkConnection ParseClientId(ulong netcodeConnectionId)
return *(NetworkConnection*)&netcodeConnectionId;
}

private void ClearSendQueuesForClientId(ulong clientId)
{
// NativeList and manual foreach avoids any allocations.
using var keys = new NativeList<SendTarget>(16, Allocator.Temp);
foreach (var key in m_SendQueue.Keys)
{
if (key.ClientId == clientId)
{
keys.Add(key);
}
}

foreach (var target in keys)
{
m_SendQueue[target].Dispose();
m_SendQueue.Remove(target);
}
}

public override void DisconnectLocalClient()
{
if (m_State == State.Connected)
{
if (m_Driver.Disconnect(ParseClientId(m_ServerClientId)) == 0)
{

m_State = State.Disconnected;

m_ReliableReceiveQueues.Remove(m_ServerClientId);
ClearSendQueuesForClientId(m_ServerClientId);

// If we successfully disconnect we dispatch a local disconnect message
// this how uNET and other transports worked and so this is just keeping with the old behavior
// should be also noted on the client this will call shutdown on the NetworkManager and the Transport
Expand All @@ -713,22 +737,8 @@ public override void DisconnectRemoteClient(ulong clientId)
m_Driver.Disconnect(connection);
}

// we need to cleanup any SendQueues for this connectionID;
var keys = new NativeList<SendTarget>(16, Allocator.Temp); // use nativelist and manual foreach to avoid allocations
foreach (var key in m_SendQueue.Keys)
{
if (key.ClientId == clientId)
{
keys.Add(key);
}
}

foreach (var queue in keys)
{
m_SendQueue[queue].Dispose();
m_SendQueue.Remove(queue);
}
keys.Dispose();
m_ReliableReceiveQueues.Remove(clientId);
ClearSendQueuesForClientId(clientId);
}
}

Expand Down
64 changes: 52 additions & 12 deletions com.unity.netcode.adapter.utp/Tests/Runtime/ConnectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ public IEnumerator ConnectSingleClient()
m_Server.StartServer();
m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

// Check we've received Connect event on client too.
Assert.AreEqual(1, m_ClientsEvents[0].Count);
Assert.AreEqual(NetworkEvent.Connect, m_ClientsEvents[0][0].Type);
// Check we've received Connect event on server too.
Assert.AreEqual(1, m_ServerEvents.Count);
Assert.AreEqual(NetworkEvent.Connect, m_ServerEvents[0].Type);

yield return null;
}
Expand All @@ -75,12 +75,16 @@ public IEnumerator ConnectMultipleClients()
m_Clients[i].StartClient();
}

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[k_NumClients - 1]);

// Check that every client also received a Connect event.
// Check that every client received a Connect event.
Assert.True(m_ClientsEvents.All(evs => evs.Count == 1));
Assert.True(m_ClientsEvents.All(evs => evs[0].Type == NetworkEvent.Connect));

// Check we've received Connect events on server too.
Assert.AreEqual(k_NumClients, m_ServerEvents.Count);
Assert.True(m_ServerEvents.All(ev => ev.Type == NetworkEvent.Connect));

yield return null;
}

Expand All @@ -94,7 +98,7 @@ public IEnumerator ServerDisconnectSingleClient()
m_Server.StartServer();
m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

m_Server.DisconnectRemoteClient(m_ServerEvents[0].ClientID);

Expand All @@ -116,7 +120,7 @@ public IEnumerator ServerDisconnectMultipleClients()
m_Clients[i].StartClient();
}

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[k_NumClients - 1]);

// Disconnect a single client.
m_Server.DisconnectRemoteClient(m_ServerEvents[0].ClientID);
Expand Down Expand Up @@ -153,7 +157,7 @@ public IEnumerator ClientDisconnectSingleClient()
m_Server.StartServer();
m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

m_Clients[0].DisconnectLocalClient();

Expand All @@ -173,7 +177,7 @@ public IEnumerator ClientDisconnectMultipleClients()
m_Clients[i].StartClient();
}

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[k_NumClients - 1]);

// Disconnect a single client.
m_Clients[0].DisconnectLocalClient();
Expand Down Expand Up @@ -205,7 +209,7 @@ public IEnumerator RepeatedServerDisconnectsNoop()
m_Server.StartServer();
m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

m_Server.DisconnectRemoteClient(m_ServerEvents[0].ClientID);

Expand Down Expand Up @@ -236,7 +240,7 @@ public IEnumerator RepeatedClientDisconnectsNoop()
m_Server.StartServer();
m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ServerEvents);
yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

m_Clients[0].DisconnectLocalClient();

Expand Down Expand Up @@ -278,5 +282,41 @@ public IEnumerator DifferentServerAndListenAddresses()

yield return null;
}

// Check that a server can disconnect a client after another client has disconnected.
[UnityTest]
public IEnumerator ServerDisconnectAfterClientDisconnect()
{
InitializeTransport(out m_Server, out m_ServerEvents);
InitializeTransport(out m_Clients[0], out m_ClientsEvents[0]);
InitializeTransport(out m_Clients[1], out m_ClientsEvents[1]);

m_Server.StartServer();

m_Clients[0].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[0]);

m_Clients[1].StartClient();

yield return WaitForNetworkEvent(NetworkEvent.Connect, m_ClientsEvents[1]);

m_Clients[0].DisconnectLocalClient();

yield return WaitForNetworkEvent(NetworkEvent.Disconnect, m_ServerEvents);

// Pick the client ID of the still connected client.
var clientId = m_ServerEvents[0].ClientID;
if (m_ServerEvents[2].ClientID == clientId)
{
clientId = m_ServerEvents[1].ClientID;
}

m_Server.DisconnectRemoteClient(clientId);

yield return WaitForNetworkEvent(NetworkEvent.Disconnect, m_ClientsEvents[1]);

yield return null;
}
}
}