Skip to content

Commit 9758607

Browse files
fix: [Backport] Ensure NetworkConnectionManager correctly handles multiple disconnect messages being sent (#3721)
* fix: [Backport] Ensure NetworkConnectionManager correctly handles multiple disconnect messages being sent * Update CHANGELOG * Simplify scripting defines in UnityTransport.GetEndpoint --------- Co-authored-by: Michał Chrobot <[email protected]>
1 parent 1ef98b8 commit 9758607

File tree

9 files changed

+164
-63
lines changed

9 files changed

+164
-63
lines changed

com.unity.netcode.gameobjects/CHANGELOG.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
1313

1414
### Changed
1515

16-
* Changed minimum Unity version supported to 2022.3 LTS
16+
- The `NetworkManager` functions `GetTransportIdFromClientId` and `GetClientIdFromTransportId` will now return `ulong.MaxValue` when the clientId or transportId do not exist. (#3721)
17+
- Changed minimum Unity version supported to 2022.3 LTS
1718

1819
### Deprecated
1920

@@ -23,6 +24,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
2324

2425
### Fixed
2526

27+
- Multiple disconnect events from the same transport will no longer disconnect the host. (#3721)
2628

2729
### Security
2830

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -317,45 +317,45 @@ internal void RemovePendingClient(ulong clientId)
317317
private ulong m_NextClientId = 1;
318318

319319
[MethodImpl(MethodImplOptions.AggressiveInlining)]
320-
internal ulong TransportIdToClientId(ulong transportId)
320+
internal (ulong, bool) TransportIdToClientId(ulong transportId)
321321
{
322322
if (transportId == GetServerTransportId())
323323
{
324-
return NetworkManager.ServerClientId;
324+
return (NetworkManager.ServerClientId, true);
325325
}
326326

327327
if (TransportIdToClientIdMap.TryGetValue(transportId, out var clientId))
328328
{
329-
return clientId;
329+
return (clientId, true);
330330
}
331331

332332
if (NetworkLog.CurrentLogLevel == LogLevel.Developer)
333333
{
334334
NetworkLog.LogWarning($"Trying to get the NGO client ID map for the transport ID ({transportId}) but did not find the map entry! Returning default transport ID value.");
335335
}
336336

337-
return default;
337+
return (0, false);
338338
}
339339

340340
[MethodImpl(MethodImplOptions.AggressiveInlining)]
341-
internal ulong ClientIdToTransportId(ulong clientId)
341+
internal (ulong, bool) ClientIdToTransportId(ulong clientId)
342342
{
343343
if (clientId == NetworkManager.ServerClientId)
344344
{
345-
return GetServerTransportId();
345+
return (GetServerTransportId(), true);
346346
}
347347

348348
if (ClientIdToTransportIdMap.TryGetValue(clientId, out var transportClientId))
349349
{
350-
return transportClientId;
350+
return (transportClientId, true);
351351
}
352352

353353
if (NetworkLog.CurrentLogLevel == LogLevel.Developer)
354354
{
355355
NetworkLog.LogWarning($"Trying to get the transport client ID map for the NGO client ID ({clientId}) but did not find the map entry! Returning default transport ID value.");
356356
}
357357

358-
return default;
358+
return (0, false);
359359
}
360360

361361
/// <summary>
@@ -384,19 +384,24 @@ private ulong GetServerTransportId()
384384
/// Handles cleaning up the transport id/client id tables after receiving a disconnect event from transport.
385385
/// </summary>
386386
[MethodImpl(MethodImplOptions.AggressiveInlining)]
387-
internal ulong TransportIdCleanUp(ulong transportId)
387+
internal (ulong, bool) TransportIdCleanUp(ulong transportId)
388388
{
389389
// This check is for clients that attempted to connect but failed.
390390
// When this happens, the client will not have an entry within the m_TransportIdToClientIdMap or m_ClientIdToTransportIdMap lookup tables so we exit early and just return 0 to be used for the disconnect event.
391391
if (!LocalClient.IsServer && !TransportIdToClientIdMap.ContainsKey(transportId))
392392
{
393-
return NetworkManager.LocalClientId;
393+
return (NetworkManager.LocalClientId, true);
394+
}
395+
396+
var (clientId, isConnectedClient) = TransportIdToClientId(transportId);
397+
if (!isConnectedClient)
398+
{
399+
return (default, false);
394400
}
395401

396-
var clientId = TransportIdToClientId(transportId);
397402
TransportIdToClientIdMap.Remove(transportId);
398403
ClientIdToTransportIdMap.Remove(clientId);
399-
return clientId;
404+
return (clientId, true);
400405
}
401406

402407
internal void PollAndHandleNetworkEvents()
@@ -502,8 +507,11 @@ internal void DataEventHandler(ulong transportClientId, ref ArraySegment<byte> p
502507
#if DEVELOPMENT_BUILD || UNITY_EDITOR
503508
s_HandleIncomingData.Begin();
504509
#endif
505-
var clientId = TransportIdToClientId(transportClientId);
506-
MessageManager.HandleIncomingData(clientId, payload, receiveTime);
510+
var (clientId, isConnectedClient) = TransportIdToClientId(transportClientId);
511+
if (isConnectedClient)
512+
{
513+
MessageManager.HandleIncomingData(clientId, payload, receiveTime);
514+
}
507515

508516
#if DEVELOPMENT_BUILD || UNITY_EDITOR
509517
s_HandleIncomingData.End();
@@ -515,10 +523,15 @@ internal void DataEventHandler(ulong transportClientId, ref ArraySegment<byte> p
515523
/// </summary>
516524
internal void DisconnectEventHandler(ulong transportClientId)
517525
{
526+
var (clientId, wasConnectedClient) = TransportIdCleanUp(transportClientId);
527+
if (!wasConnectedClient)
528+
{
529+
return;
530+
}
531+
518532
#if DEVELOPMENT_BUILD || UNITY_EDITOR
519533
s_TransportDisconnect.Begin();
520534
#endif
521-
var clientId = TransportIdCleanUp(transportClientId);
522535
if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
523536
{
524537
NetworkLog.LogInfo($"Disconnect Event From {clientId}");
@@ -1040,9 +1053,9 @@ internal void OnClientDisconnectFromServer(ulong clientId)
10401053
}
10411054

10421055
// If the client ID transport map exists
1043-
if (ClientIdToTransportIdMap.ContainsKey(clientId))
1056+
var (transportId, isConnected) = ClientIdToTransportId(clientId);
1057+
if (isConnected)
10441058
{
1045-
var transportId = ClientIdToTransportId(clientId);
10461059
NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
10471060

10481061
InvokeOnClientDisconnectCallback(clientId);

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,15 +1156,27 @@ private void HostServerInitialize()
11561156
/// Get the TransportId from the associated ClientId.
11571157
/// </summary>
11581158
/// <param name="clientId">The ClientId to get the TransportId from</param>
1159-
/// <returns>The TransportId associated with the given ClientId</returns>
1160-
public ulong GetTransportIdFromClientId(ulong clientId) => ConnectionManager.ClientIdToTransportId(clientId);
1159+
/// <returns>
1160+
/// The TransportId associated with the given ClientId if the given clientId is valid; otherwise <see cref="ulong.MaxValue"/>
1161+
/// </returns>
1162+
public ulong GetTransportIdFromClientId(ulong clientId)
1163+
{
1164+
var (id, success) = ConnectionManager.ClientIdToTransportId(clientId);
1165+
return success ? id : ulong.MaxValue;
1166+
}
11611167

11621168
/// <summary>
11631169
/// Get the ClientId from the associated TransportId.
11641170
/// </summary>
11651171
/// <param name="transportId">The TransportId to get the ClientId from</param>
1166-
/// <returns>The ClientId from the associated TransportId</returns>
1167-
public ulong GetClientIdFromTransportId(ulong transportId) => ConnectionManager.TransportIdToClientId(transportId);
1172+
/// <returns>
1173+
/// The ClientId from the associated TransportId if the given transportId is valid; otherwise <see cref="ulong.MaxValue"/>
1174+
/// </returns>
1175+
public ulong GetClientIdFromTransportId(ulong transportId)
1176+
{
1177+
var (id, success) = ConnectionManager.TransportIdToClientId(transportId);
1178+
return success ? id : ulong.MaxValue;
1179+
}
11681180

11691181
/// <summary>
11701182
/// Disconnects the remote client.

com.unity.netcode.gameobjects/Runtime/Messaging/DefaultMessageSender.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,19 @@ public DefaultMessageSender(NetworkManager manager)
1919
public void Send(ulong clientId, NetworkDelivery delivery, FastBufferWriter batchData)
2020
{
2121
var sendBuffer = batchData.ToTempByteArray();
22+
var (transportId, clientExists) = m_ConnectionManager.ClientIdToTransportId(clientId);
2223

23-
m_NetworkTransport.Send(m_ConnectionManager.ClientIdToTransportId(clientId), sendBuffer, delivery);
24+
if (!clientExists)
25+
{
26+
if (m_ConnectionManager.NetworkManager.LogLevel <= LogLevel.Error)
27+
{
28+
NetworkLog.LogWarning("Trying to send a message to a client who doesn't have a transport connection");
29+
}
30+
31+
return;
32+
}
33+
34+
m_NetworkTransport.Send(transportId, sendBuffer, delivery);
2435
}
2536
}
2637
}

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

Lines changed: 19 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -868,7 +868,11 @@ private void SendBatchedMessages(SendTarget sendTarget, BatchedSendQueue queue)
868868
var mtu = 0;
869869
if (NetworkManager)
870870
{
871-
var ngoClientId = NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
871+
var (ngoClientId, isConnectedClient) = NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
872+
if (!isConnectedClient)
873+
{
874+
return;
875+
}
872876
mtu = NetworkManager.GetPeerMTU(ngoClientId);
873877
}
874878

@@ -1278,7 +1282,7 @@ public override ulong GetCurrentRtt(ulong clientId)
12781282

12791283
if (NetworkManager != null)
12801284
{
1281-
var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
1285+
var (transportId, _) = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
12821286

12831287
var rtt = ExtractRtt(ParseClientId(transportId));
12841288
if (rtt > 0)
@@ -1290,7 +1294,6 @@ public override ulong GetCurrentRtt(ulong clientId)
12901294
return (ulong)ExtractRtt(ParseClientId(clientId));
12911295
}
12921296

1293-
#if UTP_TRANSPORT_2_0_ABOVE
12941297
/// <summary>
12951298
/// Provides the <see cref="NetworkEndpoint"/> for the NGO client identifier specified.
12961299
/// </summary>
@@ -1305,40 +1308,19 @@ public NetworkEndpoint GetEndpoint(ulong clientId)
13051308
{
13061309
if (m_Driver.IsCreated && NetworkManager != null && NetworkManager.IsListening)
13071310
{
1308-
var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
1311+
var (transportId, connectionExists) = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
13091312
var networkConnection = ParseClientId(transportId);
1310-
if (m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected)
1313+
if (connectionExists && m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected)
13111314
{
1315+
#if UTP_TRANSPORT_2_0_ABOVE
13121316
return m_Driver.GetRemoteEndpoint(networkConnection);
1313-
}
1314-
}
1315-
return new NetworkEndpoint();
1316-
}
13171317
#else
1318-
/// <summary>
1319-
/// Provides the <see cref="NetworkEndpoint"/> for the NGO client identifier specified.
1320-
/// </summary>
1321-
/// <remarks>
1322-
/// - This is only really useful for direct connections.
1323-
/// - Relay connections and clients connected using a distributed authority network topology will not provide the client's actual endpoint information.
1324-
/// - For LAN topologies this should work as long as it is a direct connection and not a relay connection.
1325-
/// </remarks>
1326-
/// <param name="clientId">NGO client identifier to get endpoint information about.</param>
1327-
/// <returns><see cref="NetworkEndpoint"/></returns>
1328-
public NetworkEndpoint GetEndpoint(ulong clientId)
1329-
{
1330-
if (m_Driver.IsCreated && NetworkManager != null && NetworkManager.IsListening)
1331-
{
1332-
var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
1333-
var networkConnection = ParseClientId(transportId);
1334-
if (m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected)
1335-
{
13361318
return m_Driver.RemoteEndPoint(networkConnection);
1319+
#endif
13371320
}
13381321
}
13391322
return new NetworkEndpoint();
13401323
}
1341-
#endif
13421324

13431325

13441326
/// <summary>
@@ -1460,10 +1442,17 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel
14601442
// If the message is sent reliably, then we're over capacity and we can't
14611443
// provide any reliability guarantees anymore. Disconnect the client since at
14621444
// this point they're bound to become desynchronized.
1445+
if (NetworkManager != null)
1446+
{
1447+
var (ngoClientId, isConnectedClient) = NetworkManager.ConnectionManager.TransportIdToClientId(clientId);
1448+
if (isConnectedClient)
1449+
{
1450+
clientId = ngoClientId;
1451+
}
14631452

1464-
var ngoClientId = NetworkManager?.ConnectionManager.TransportIdToClientId(clientId) ?? clientId;
1453+
}
14651454
Debug.LogError($"Couldn't add payload of size {payload.Count} to reliable send queue. " +
1466-
$"Closing connection {ngoClientId} as reliability guarantees can't be maintained.");
1455+
$"Closing connection {clientId} as reliability guarantees can't be maintained.");
14671456

14681457
if (clientId == m_ServerClientId)
14691458
{

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,17 @@ public enum HostOrServer
287287
/// </summary>
288288
protected virtual bool m_EnableTimeTravel => false;
289289

290+
/// <summary>
291+
/// When true, <see cref="CreateServerAndClients()"/> and <see cref="CreateNewClient"/> will use a <see cref="MockTransport"/>
292+
/// as the <see cref="NetworkConfig.NetworkTransport"/> on the created server and/or clients.
293+
/// When false, a <see cref="UnityTransport"/> is used.
294+
/// </summary>
295+
/// <remarks>
296+
/// This defaults to, and is required to be true when <see cref="m_EnableTimeTravel"/> is true.
297+
/// <see cref="m_EnableTimeTravel"/> will not work with the <see cref="UnityTransport"/> component.
298+
/// </remarks>
299+
protected virtual bool m_UseMockTransport => m_EnableTimeTravel;
300+
290301
/// <summary>
291302
/// If this is false, SetUp will call OnInlineSetUp instead of OnSetUp.
292303
/// This is a performance advantage when not using the coroutine functionality, as a coroutine that
@@ -407,7 +418,7 @@ public IEnumerator SetUp()
407418
{
408419
VerboseDebug($"Entering {nameof(SetUp)}");
409420
NetcodeLogAssert = new NetcodeLogAssert();
410-
if (m_EnableTimeTravel)
421+
if (m_UseMockTransport)
411422
{
412423
if (m_NetworkManagerInstatiationMode == NetworkManagerInstatiationMode.AllTests)
413424
{
@@ -417,9 +428,14 @@ public IEnumerator SetUp()
417428
{
418429
MockTransport.Reset();
419430
}
420-
// Setup the frames per tick for time travel advance to next tick
431+
}
432+
433+
// Setup the frames per tick for time travel advance to next tick
434+
if (m_EnableTimeTravel)
435+
{
421436
ConfigureFramesPerTick();
422437
}
438+
423439
if (m_SetupIsACoroutine)
424440
{
425441
yield return OnSetup();
@@ -577,7 +593,7 @@ protected virtual bool ShouldWaitForNewClientToConnect(NetworkManager networkMan
577593
/// <returns>An IEnumerator for use with Unity's coroutine system.</returns>
578594
protected IEnumerator CreateAndStartNewClient()
579595
{
580-
var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_EnableTimeTravel);
596+
var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_UseMockTransport);
581597
networkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
582598

583599
// Notification that the new client (NetworkManager) has been created
@@ -619,7 +635,7 @@ protected IEnumerator CreateAndStartNewClient()
619635
/// </summary>
620636
protected void CreateAndStartNewClientWithTimeTravel()
621637
{
622-
var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_EnableTimeTravel);
638+
var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_UseMockTransport);
623639
networkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
624640

625641
// Notification that the new client (NetworkManager) has been created
@@ -732,7 +748,7 @@ protected void CreateServerAndClients(int numberOfClients)
732748
}
733749

734750
// Create multiple NetworkManager instances
735-
if (!NetcodeIntegrationTestHelpers.Create(numberOfClients, out NetworkManager server, out NetworkManager[] clients, m_TargetFrameRate, m_CreateServerFirst, m_EnableTimeTravel))
751+
if (!NetcodeIntegrationTestHelpers.Create(numberOfClients, out NetworkManager server, out NetworkManager[] clients, m_TargetFrameRate, m_CreateServerFirst, m_UseMockTransport))
736752
{
737753
Debug.LogError("Failed to create instances");
738754
Assert.Fail("Failed to create instances");
@@ -1219,7 +1235,7 @@ public IEnumerator TearDown()
12191235
VerboseDebug($"Exiting {nameof(TearDown)}");
12201236
LogWaitForMessages();
12211237
NetcodeLogAssert.Dispose();
1222-
if (m_EnableTimeTravel)
1238+
if (m_UseMockTransport)
12231239
{
12241240
if (m_NetworkManagerInstatiationMode == NetworkManagerInstatiationMode.AllTests)
12251241
{

0 commit comments

Comments
 (0)