fix: NetworkManager ApprovalTimeout should not depend upon client synchronization #2261
Conversation
This file contains 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 is the final update for the ApprovalTimeout updates. It accounts for latency and logs more meaningful information for the users. The latency adjustments are capped at no more than 1 second longer than the specified ClientConnectionBufferTimeout to prevent from latency derived attacks.
Added the ability to bypass the entire NetcodeIntegrationTest connection approval process after the server and clients have been started. This allows us to now use NetcodeIntegrationTest for unique connection oriented tests without being bound to the asserts if not all clients connected properly.
This is a bit of a refactoring for ConnectionApprovalTimeoutTests. It uses the added m_BypassConnectionTimeout to bypass the waiting for clients to connect. It still uses the message hook catch technique to simulate the timeout scenarios where either a server detects a transport connection but never receives a connection request or a client sends the connection request but never receives approval for the connection.
| @@ -232,6 +232,10 @@ private void BuildDestinationToTransitionInfoTable() | |||
| private void BuildTransitionStateInfoList() | |||
| { | |||
| #if UNITY_EDITOR | |||
| if (UnityEditor.EditorApplication.isUpdating) | |||
There was a problem hiding this comment.
This might get removed and placed into a really small PR, but it seems to fix the "domain backup" errors we have been seeing during serialization with the more recent versions of Unity (a known issue).
updating some of the names and comments.
jeffreyrainy
approved these changes
Oct 18, 2022
NoelStephensUnity
deleted the
fix/separate-approval-from-clientconnected
branch
jakobbbb
pushed a commit
to GooseGirlGames/com.unity.netcode.gameobjects
that referenced
this pull request
Feb 22, 2023
…chronization (Unity-Technologies#2261) * fix This fix separates the IsConnectedClient from the approval process by adding an IsApproved flag. This also has a fix to prevent the domain backup error during serialization. * test Added the ability to bypass the entire NetcodeIntegrationTest connection approval process after the server and clients have been started. This allows us to now use NetcodeIntegrationTest for unique connection oriented tests without being bound to the asserts if not all clients connected properly. Refactored for ConnectionApprovalTimeoutTests to use the added m_BypassConnectionTimeout to bypass the waiting for clients to connect. It still uses the message hook catch technique to simulate the timeout scenarios where either a server detects a transport connection but never receives a connection request or a client sends the connection request but never receives approval for the connection.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This bug was discovered in a Boss Room play test where the ClientConnectionBufferTimeout was set to 1 second. The underlying issue was that the ApprovalTimeout coroutine would depend upon the NetworkManager.IsConnectedClient to be determine if the client has been approved. This is problematic because NetworkManager.IsConnectedClient (when scene management is enabled) is set once the client has already been approved and has been fully synchronized. If the time it takes a client to synchronize exceeds the ClientConnectionBufferTimeout then while the client was approved it would time out anyway.
Here is how this fix addresses the issue:
MTT-4895
Changelog
NetworkManager.IsApprovedflag that is set totruea client has been approved.NetworkManager.ApprovalTimeoutwill not timeout due to slower client synchronization times as it now uses the addedNetworkManager.IsApprovedflag to determined if the client has been approved or not.Testing and Documentation