Skip to content
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

fix: networkvariable collections can be modified without write permissions and more #3081

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Sep 27, 2024

This PR resolves the issue where a client or server could modify a collection with no write permissions. The underlying issue is that default .NET collections have no way to be notified when the collection is modified. Since polling each instance is not an option, the lowest risk fix that continues to support standard .NET collections was to:

  • Add a 3rd internal duplicated current value property that can be used to compare against the current internal value reading (applying) a delta or field value. This helps to avoid edge case scenarios where the current collection value on a client with no write permissions could be modified and a state update is received.
  • Update NetworkVariable.CheckDirtyState and NetworkVariable.IsDirty such that the same form of comparison occurs to provide users with an additional way to check collection integrity on clients without write permissions while also being able to control how often the collection is checked.

This PR also resolves:

  • The issue where when one or more clients is/are already connected, entries are added by the current owner, the ownership changes to one of the connected clients, and finally the new client owner adds an entry. Since the new client's previous value always lags behind the last update, the new client will send both the new entry and the previously added entry which causes an exception on non-owner clients (duplicate key entry) and/or additional duplicate entries could be added (i.e. a list).
  • The issue where changing ownership would mark all NetworkVariables dirty as opposed to:
    • Marking any owner read permissions NetworkVariables as dirty when sending to the new owner (both within NetworkSpawnManager for server-side update and within the NetworkVariableDeltaMessage).
    • Sending any pending updates to all clients prior to sending the change in ownership message to assure pending updates are synchronized as the owner.
  • The issue where a newly synchronizing client would be synchronized with the current NetworkVariable values always which could cause issues with collections if there were any pending state updates. Now, when initially synchronizing a client, if a NetworkVariable has a pending state update it will serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they don't duplicate values.

This PR also changes:
NetworkVariableDeltaMessage now includes the ability for a server to immediately forward delta state updates (to valid clients on a per NetworkVariable field basis) without having to queue them for end of frame (i.e. since messages are processed at the beginning of a frame, this shaves off the state replication time to other clients by close to the total frame time...which depending upon your project could mean 10-16ms faster).

fix: #3070

Changelog

  • Changed: NetworkVariableDeltaMessage so the server now forwards delta state updates (owner write permission based from a client) to other clients immediately as opposed to keeping a NetworkVariable or NetworkList dirty and processing them at the end of the frame or potentially on the next network tick.
  • Fixed: Issue where a newly synchronizing client would be synchronized with the current NetworkVariable values always which could cause issues with collections if there were any pending state updates. Now, when initially synchronizing a client, if a NetworkVariable has a pending state update it will serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they aren't duplicate values on the newly connected client side.
  • Fixed: Issue where changing ownership would mark every NetworkVariable dirty. Now, it will only mark any NetworkVariable with owner read permissions as dirty and will send/flush any pending updates to all clients prior to sending the change in ownership message.
  • Fixed: Issue with NetworkVariable collections where transferring ownership to another client would not update the new owner's previous value to the most current value which could cause the last/previous added value to be detected as a change when adding or removing an entry (as long as the entry removed was not the last/previously added value).
  • Fixed: Issue where a client (or server) with no write permissions for a NetworkVariable using a standard .NET collection type could still modify the collection which could cause various issues depending upon the modification and collection type.

Testing and Documentation

  • Includes integration test updates to NetworkVariableCollectionsTests.
  • Some public documentation updates required (internal tracking added).

Provide additional copy of last known current NetworkVariable.Value to be able to compare against the current local value in order to detect if a client without write permissions has modified a collection.
Updated collections validation tests to spot check the restore known current state when client without write permissions modifies a collection.
@NoelStephensUnity NoelStephensUnity added the type:backport-release-1.0.0 This PR should be backported to 1.0.0 label Sep 27, 2024
added changelog entry
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review September 27, 2024 22:39
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner September 27, 2024 22:39
Changing order of operations within CheckDirtyState for clients without write permissions
Add some spot checks to one of the dictionary tests
Fixing issue where upon a client gaining ownership of a NetworkObject that has one or more owner write permission NetworkVariables using a collection type with keyed indices (like a dictionary) can resend already synchronized entries when making a change to the collection causing non-owner clients to throw a key already exists exception.
Adding changelog entrry
Adjusted when the NetworkVariable update for ownership change is invoked to account for updates to owner write NetworkVariables within the OnGainedOwnership callback.

When changing ownership:
- Marking any owner read permissions NetworkVariables as dirty when sending to the new owner (both within NetworkSpawnManager for server-side update and within the NetworkVariableDeltaMessage).
- Sending any pending updates to all clients prior to sending the change in ownership message to assure pending updates are synchronized as the owner.

When initially synchronizing a client, if a NetworkVariable has a pending state update then send serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they don't duplicate values.
Adjusting two deferred message tests to not account for a NetworkVariable delta state update message when changing ownership and there are no pending updates.
updating changelog entries
@NoelStephensUnity NoelStephensUnity changed the title fix: networkvariable collections can be modified without write permissions fix: networkvariable collections can be modified without write permissions and more Sep 30, 2024
This includes additional fixes for NetworkVariable collections that ended up requiring a different approach to how a server forwards NetworkVariable deltas. Now, NetworkVariableDeltaMessage forwards NetworkVariable field deltas immediately after a server has finished processing the deltas (i.e. the keeping a NetworkVariable dirty concept is not used from this point forward). I went ahead and kept the compatibility of this functionality.

NetworkVariableDeltaMessage has had its Version incremented as we now send the NetworkDelivery type in order to be able to handle this (it seemed less risky to include this than to try and bubble up this property to all message types).

This also separates the duplication of the "original internal value" and the "previous value" until after processing all deltas. If the server has to foward, it forwards first then invokes the PostDeltaRead method that performs the duplication.
This includes some minor adjustments to NetworkList in order to adjust to this new order of operations while also preserving the legacy approach.

This also includes some adjustments to NetworkBehaviourUpdater where it can force a send (flush) any pending dirty fields when changing ownership. This includes some minor modifications to NetworkObject.PostNetworkVariableWrite.
Just some updates to two integration tests. These are all primarily for debugging purposes.
Adding last change log entry for this PR.
Updated a changelog entry to make it clearer.
DAHost fixes:
Fixed issue where it was possible to ignore forwarding a change in ownership message to the destination owner if the original owner changed ownership.
Fixed issue where it was possible to re-send a change in ownership message back to the original owner if the original owner sent the message.
Adding and additional test that validates several of the fixes in this PR.
NoelStephensUnity and others added 2 commits October 16, 2024 13:23
Removing the DAHost testfixture that wasn't supposed to be added.
Fixing an issue with the NetworkObjectOwnershipTests when running in DAHost mode. It was passing because the DAHost was sending the ChangeOwnershipMessage back to the owner that changed ownership to another client (without the fix for that in this PR it would send the message to the authority/owner which is why the test was passing).
Updating all instances of k_ServerDeltaForwadingAndNetworkDelivery and replacing with k_ServerDeltaForwardingAndNetworkDelivery
@NoelStephensUnity NoelStephensUnity merged commit 632bf35 into develop-2.0.0 Oct 16, 2024
24 checks passed
@NoelStephensUnity NoelStephensUnity deleted the fix/networkvariable-collections-can-be-modified-without-write-permissions branch October 16, 2024 20:24
NoelStephensUnity added a commit that referenced this pull request Nov 15, 2024
Backporting #3081 to NGO v1
NoelStephensUnity added a commit that referenced this pull request Nov 15, 2024
Back porting #3081 test updates (where applicable).
NoelStephensUnity added a commit that referenced this pull request Nov 19, 2024
…sions [backport] (#3126)

* Update

Backporting #3081 to NGO v1

* test

Back porting #3081 test updates (where applicable).

* update

adding changelog entries

* update

Disposing the internal original.

* fix

Making sure the force send is passed along to the NetworkBehaviour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:backport-release-1.0.0 This PR should be backported to 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants