test: manual connection approval to fully automated connection approval testing #839
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,15 +39,6 @@ private class Peer | |
|
|
||
| public override void DisconnectLocalClient() | ||
| { | ||
| if (m_LocalConnection != null) | ||
| { | ||
| // Inject local disconnect | ||
|
|
@@ -78,29 +69,32 @@ public override void DisconnectLocalClient() | |
| // Called by server | ||
| public override void DisconnectRemoteClient(ulong clientId) | ||
| { | ||
| if (m_Clients.ContainsKey(clientId)) | ||
| { | ||
| // Inject disconnect into remote | ||
| m_Clients[clientId].IncomingBuffer.Enqueue(new Event | ||
| { | ||
| Type = NetworkEvent.Disconnect, | ||
| Channel = NetworkChannel.Internal, | ||
| ConnectionId = clientId, | ||
| Data = new ArraySegment<byte>() | ||
| }); | ||
|
|
||
| // Inject local disconnect | ||
| m_LocalConnection.IncomingBuffer.Enqueue(new Event | ||
| { | ||
| Type = NetworkEvent.Disconnect, | ||
| Channel = NetworkChannel.Internal, | ||
| ConnectionId = clientId, | ||
| Data = new ArraySegment<byte>() | ||
| }); | ||
|
|
||
| // Remove the local connection on remote | ||
| m_Clients[clientId].Transport.m_LocalConnection = null; | ||
|
|
||
| // Remove connection on server | ||
| m_Clients.Remove(clientId); | ||
| } | ||
| } | ||
|
|
||
| public override ulong GetCurrentRtt(ulong clientId) | ||
|
|
@@ -220,38 +214,52 @@ public override SocketTasks StartServer() | |
|
|
||
| public override void Send(ulong clientId, ArraySegment<byte> data, NetworkChannel channel) | ||
| { | ||
| if (m_LocalConnection != null) | ||
| { | ||
| // Create copy since MLAPI wants the byte array back straight after the method call. | ||
| // Hard on GC. | ||
| byte[] copy = new byte[data.Count]; | ||
| Buffer.BlockCopy(data.Array, data.Offset, copy, 0, data.Count); | ||
|
|
||
| m_Clients[clientId].IncomingBuffer.Enqueue(new Event | ||
| { | ||
| Type = NetworkEvent.Data, | ||
| ConnectionId = m_LocalConnection.ConnectionId, | ||
| Data = new ArraySegment<byte>(copy), | ||
| Channel = channel | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| public override NetworkEvent PollEvent(out ulong clientId, out NetworkChannel channel, out ArraySegment<byte> payload, out float receiveTime) | ||
| { | ||
| if (m_LocalConnection != null) | ||
|
Member
Author
There was a problem hiding this comment. If a player is disconnected m_LocalConnection becomes null, but can still be called since the NetworkManager is not notified of the client disconnecting. The resolution for this was to return back NetworkEvent.Nothing for the still running NetworkManager that is attempting to poll the SIPTransport for the client in question.
Contributor
There was a problem hiding this comment. similar to above, I'd warn dev here and even expect them to look at their call-stack & code-path when this happens.
Member
Author
There was a problem hiding this comment. @TwoTenPvP Please review the above changes. |
||
| { | ||
| if (m_LocalConnection.IncomingBuffer.Count == 0) | ||
| { | ||
| clientId = 0; | ||
| channel = NetworkChannel.Internal; | ||
| payload = new ArraySegment<byte>(); | ||
| receiveTime = 0; | ||
| return NetworkEvent.Nothing; | ||
| } | ||
|
|
||
| var peerEvent = m_LocalConnection.IncomingBuffer.Dequeue(); | ||
|
|
||
| clientId = peerEvent.ConnectionId; | ||
| channel = peerEvent.Channel; | ||
| payload = peerEvent.Data; | ||
| receiveTime = 0; | ||
|
|
||
| return peerEvent.Type; | ||
| } | ||
| else | ||
| { | ||
| clientId = 0; | ||
| channel = NetworkChannel.Internal; | ||
| payload = new ArraySegment<byte>(); | ||
| receiveTime = 0; | ||
| return NetworkEvent.Nothing; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,230 @@ | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
| using NUnit.Framework; | ||
| using UnityEngine; | ||
| using UnityEngine.TestTools; | ||
| using MLAPI.RuntimeTests; | ||
| using MLAPI; | ||
| using Debug = UnityEngine.Debug; | ||
|
|
||
| namespace TestProject.RuntimeTests | ||
| { | ||
| public class MultiClientConnectionApproval | ||
| { | ||
| private string m_ConnectionToken; | ||
| private uint m_SuccessfulConnections; | ||
| private uint m_FailedConnections; | ||
| private uint m_PrefabOverrideGlobalObjectIdHash; | ||
|
|
||
| private GameObject m_PlayerPrefab; | ||
| private GameObject m_PlayerPrefabOverride; | ||
|
|
||
| private int m_OriginalTargetFrameRate; | ||
|
|
||
| [SetUp] | ||
| public void SetUp() | ||
| { | ||
| // Just always track the current target frame rate (will be re-applied upon TearDown) | ||
| m_OriginalTargetFrameRate = Application.targetFrameRate; | ||
|
|
||
| // Since we use frame count as a metric, we need to assure it runs at a "common update rate" | ||
| // between platforms (i.e. Ubuntu seems to run at much higher FPS when set to -1) | ||
| if (Application.targetFrameRate < 0 || Application.targetFrameRate > 120) | ||
| { | ||
| Application.targetFrameRate = 120; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tests connection approval and connection approval failure | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| [UnityTest] | ||
| public IEnumerator ConnectionApproval() | ||
| { | ||
| m_ConnectionToken = "ThisIsTheRightPassword"; | ||
| return ConnectionApprovalHandler(3,1); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tests player prefab overriding, connection approval, and connection approval failure | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| [UnityTest] | ||
| public IEnumerator ConnectionApprovalPrefabOverride() | ||
| { | ||
| m_ConnectionToken = "PrefabOverrideCorrectPassword"; | ||
| return ConnectionApprovalHandler(3, 1, true); | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Allows for several connection approval related configurations | ||
| /// </summary> | ||
| /// <param name="numClients">total number of clients (excluding the host)</param> | ||
| /// <param name="failureTestCount">how many clients are expected to fail</param> | ||
| /// <param name="prefabOverride">if we are also testing player prefab overrides</param> | ||
| /// <returns></returns> | ||
| private IEnumerator ConnectionApprovalHandler(int numClients, int failureTestCount = 1, bool prefabOverride = false) | ||
| { | ||
| Debug.Log($"Application.targetFrameRate = {Application.targetFrameRate}"); | ||
| if (Application.targetFrameRate == -1 || Application.targetFrameRate > 120) | ||
| { | ||
| Application.targetFrameRate = 120; | ||
| } | ||
|
|
||
| var startFrameCount = Time.frameCount; | ||
| var startTime = Time.realtimeSinceStartup; | ||
|
|
||
| m_SuccessfulConnections = 0; | ||
| m_FailedConnections = 0; | ||
| Assert.IsTrue(numClients >= failureTestCount); | ||
|
|
||
| // Create Host and (numClients) clients | ||
| Assert.True(MultiInstanceHelpers.Create(numClients, out NetworkManager server, out NetworkManager[] clients)); | ||
|
|
||
| // Create a default player GameObject to use | ||
| m_PlayerPrefab = new GameObject("Player"); | ||
| var networkObject = m_PlayerPrefab.AddComponent<NetworkObject>(); | ||
|
|
||
| // Make it a prefab | ||
| MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(networkObject); | ||
|
|
||
| // Create the player prefab override if set | ||
| if (prefabOverride) | ||
| { | ||
| // Create a default player GameObject to use | ||
| m_PlayerPrefabOverride = new GameObject("PlayerPrefabOverride"); | ||
| var networkObjectOverride = m_PlayerPrefabOverride.AddComponent<NetworkObject>(); | ||
| MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(networkObjectOverride); | ||
| m_PrefabOverrideGlobalObjectIdHash = networkObjectOverride.GlobalObjectIdHash; | ||
| } | ||
| else | ||
| { | ||
| m_PrefabOverrideGlobalObjectIdHash = 0; | ||
| } | ||
|
|
||
| // [Host-Side] Set the player prefab | ||
| server.NetworkConfig.PlayerPrefab = m_PlayerPrefab; | ||
| server.NetworkConfig.ConnectionApproval = true; | ||
| server.ConnectionApprovalCallback += ConnectionApprovalCallback; | ||
| server.NetworkConfig.ConnectionData = Encoding.ASCII.GetBytes(m_ConnectionToken); | ||
|
|
||
| // [Client-Side] Get all of the RpcQueueManualTests instances relative to each client | ||
| var clientsAdjustedList = new List<NetworkManager>(); | ||
| var clientsToClean = new List<NetworkManager>(); | ||
| var markedForFailure = 0; | ||
|
|
||
| foreach (var client in clients) | ||
| { | ||
| client.NetworkConfig.PlayerPrefab = m_PlayerPrefab; | ||
| client.NetworkConfig.ConnectionApproval = true; | ||
| if (markedForFailure < failureTestCount) | ||
| { | ||
| client.NetworkConfig.ConnectionData = Encoding.ASCII.GetBytes("ThisIsTheWrongPassword"); | ||
| markedForFailure++; | ||
| clientsToClean.Add(client); | ||
| } | ||
| else | ||
| { | ||
| client.NetworkConfig.ConnectionData = Encoding.ASCII.GetBytes(m_ConnectionToken); | ||
| clientsAdjustedList.Add(client); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // Start the instances | ||
| if (!MultiInstanceHelpers.Start(true, server, clients)) | ||
| { | ||
| Debug.LogError("Failed to start instances"); | ||
| Assert.Fail("Failed to start instances"); | ||
| } | ||
|
|
||
| // [Client-Side] Wait for a connection to the server | ||
| yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(clientsAdjustedList.ToArray(), null, 512)); | ||
|
|
||
| // [Host-Side] Check to make sure all clients are connected | ||
| yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnectedToServer(server, clientsAdjustedList.Count + 1, null, 512)); | ||
|
|
||
| // Validate the number of failed connections is the same as expected | ||
| Assert.IsTrue(m_FailedConnections == failureTestCount); | ||
|
|
||
| // Validate the number of successful connections is the total number of expected clients minus the failed client count | ||
| Assert.IsTrue(m_SuccessfulConnections == (numClients + 1) - failureTestCount); | ||
|
|
||
| // If we are doing player prefab overrides, then check all of the players to make sure they spawned the appropriate NetworkObject | ||
| if (prefabOverride) | ||
| { | ||
| foreach(var networkClient in server.ConnectedClientsList) | ||
| { | ||
| Assert.IsNotNull(networkClient.PlayerObject); | ||
| Assert.AreEqual(networkClient.PlayerObject.GlobalObjectIdHash, m_PrefabOverrideGlobalObjectIdHash); | ||
| } | ||
| } | ||
|
|
||
| foreach (var client in clients) | ||
| { | ||
| client.StopClient(); | ||
| } | ||
|
|
||
| server.ConnectionApprovalCallback -= ConnectionApprovalCallback; | ||
| server.StopHost(); | ||
|
|
||
| Debug.Log($"Application.targetFrameRate = {Application.targetFrameRate}."); | ||
| Debug.Log($"Total frames updated = {Time.frameCount - startFrameCount} within {Time.realtimeSinceStartup - startTime} seconds."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Delegate handler for the connection approval callback | ||
| /// </summary> | ||
| /// <param name="connectionData">the NetworkConfig.ConnectionData sent from the client being approved</param> | ||
| /// <param name="clientId">the client id being approved</param> | ||
| /// <param name="callback">the callback invoked to handle approval</param> | ||
| private void ConnectionApprovalCallback(byte[] connectionData, ulong clientId, NetworkManager.ConnectionApprovedDelegate callback) | ||
| { | ||
| string approvalToken = Encoding.ASCII.GetString(connectionData); | ||
| var isApproved = approvalToken == m_ConnectionToken; | ||
|
|
||
| if(isApproved) | ||
| { | ||
| m_SuccessfulConnections++; | ||
| } | ||
| else | ||
| { | ||
| m_FailedConnections++; | ||
| } | ||
|
|
||
| if (m_PrefabOverrideGlobalObjectIdHash == 0) | ||
| { | ||
| callback.Invoke(true, null, isApproved, null, null); | ||
| } | ||
| else | ||
| { | ||
| callback.Invoke(true, m_PrefabOverrideGlobalObjectIdHash, isApproved, null, null); | ||
| } | ||
| } | ||
|
|
||
| [TearDown] | ||
| public void TearDown() | ||
| { | ||
| if (m_PlayerPrefab != null) | ||
| { | ||
| Object.Destroy(m_PlayerPrefab); | ||
| m_PlayerPrefab = null; | ||
| } | ||
|
|
||
| if (m_PlayerPrefabOverride != null) | ||
| { | ||
| Object.Destroy(m_PlayerPrefabOverride); | ||
| m_PlayerPrefabOverride = null; | ||
| } | ||
|
|
||
| // Shutdown and clean up both of our NetworkManager instances | ||
| MultiInstanceHelpers.Destroy(); | ||
|
|
||
| // Set the application's target frame rate back to its original value | ||
| Application.targetFrameRate = m_OriginalTargetFrameRate; | ||
| } | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a player is disconnected m_LocalConnection becomes null, but can still be called since the NetworkManager is not notified of the client disconnecting. This resolves this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this "skip if null" logic might simply make us blind to some issues happening in the future.
please log a warning on the else branch at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TwoTenPvP Please review the above changes.
SIPTransport.DisconnectLocalClient sets m_LocalConnection to null but never notifies the relative NetworkManager that the client has disconnected which can cause other systems (i.e. SnapShotSystem) to cause a barrage of exceptions when trying to send snapshots from a disconnected client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is meant to notify all peers that it disconnected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this would happen or what's going on behind the scenes but I still want to see a warning log printed so that a dev wouldn't be blind and execution wouldn't fail silently. do you guys think my request is still reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me DM you on this... there are other things to consider here.