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 [backport] #3126

Conversation

NoelStephensUnity
Copy link
Collaborator

This is a backport of #3081.

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).

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

Backporting #3081 to NGO v1
Back porting #3081 test updates (where applicable).
adding changelog entries
Disposing the internal original.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review November 18, 2024 17:18
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner November 18, 2024 17:18
/// until it is done serializing all valid NetworkVariable field deltas (relative to each client). This is invoked
/// after it is done forwarding the deltas at the end of the <see cref="NetworkVariableDeltaMessage.Handle(ref NetworkContext)"/> method.
/// </summary>
internal virtual void PostDeltaRead()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
The two derived classes (NetworkVariable and NetworkList) provide overrides to handle this, but the base class doesn't need to do anything since it is just an abstract class.

@@ -1725,7 +1735,7 @@ public void Deserialize(FastBufferReader reader)
}
}

internal void PostNetworkVariableWrite()
internal void PostNetworkVariableWrite(bool forceSend)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method inherited? If not why is the argument added? Should it be passed on to the ChildNetworkBehaviours in the loop?

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch you are right about passing it along... that needs to be fixed.

Making sure the force send is passed along to the NetworkBehaviour
@NoelStephensUnity NoelStephensUnity merged commit 0c32e93 into develop Nov 19, 2024
24 checks passed
@NoelStephensUnity NoelStephensUnity deleted the fix/networkvariable-collections-can-be-modified-without-write-permissions-backport branch November 19, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants