Naked delegates should have event modifier keyword

[StephenHodgson]

com.unity.netcode.gameobjects/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs

Line 23 in dcaeef9

public OnValueChangedDelegate OnValueChanged;

Delegate events like the one shown above should have event keyword modifiers.

 public event OnValueChangedDelegate OnValueChanged; 

---

A "naked" delegate refers to a delegate that is declared without the event keyword. This means the delegate is exposed directly, allowing external code to not only subscribe and unsubscribe to it but also to reassign or invoke it directly. This can lead to unintended side effects and potential bugs.

For example, in the provided code snippet, the delegates are correctly declared as events:

public event Action OnSpawnedLocal;
public event Action OnSpawnedAll;
public event Action<NetworkPlayer> OnDisconnected;

If these were declared as naked delegates, they would look like this:

public Action OnSpawnedLocal;
public Action OnSpawnedAll;
public Action<NetworkPlayer> OnDisconnected;

Issues with naked delegates:

1. Reassignment: External code can reassign the delegate, potentially removing all existing subscribers.

   networkPlayer.OnSpawnedLocal = SomeMethod; // Reassigns the delegate, removing all previous subscribers.

2. Invocation: It breaks encapsulation, making it harder to control how and when the delegate is invoked.

   networkPlayer.OnSpawnedLocal?.Invoke(); // Invokes the delegate from outside the class.

3. Dangling Pointers: Events in C# are multicast delegates which can have multiple and anonymous methods subscribe to the same event. Using the event keyword will help properly manage those subscriptions and unsubscriptions. This way if someone forgets to unsubscribe, the dangling pointer will get properly cleaned up by garbage collector.

• If a delegate holds a reference to an object that should be garbage collected, it can prevent the object from being collected, leading to memory leaks.
• If the target method or object of a delegate has been collected, invoking the delegate can result in a NullReferenceException or other runtime errors.

TR;DL:

I've seen this throughout the whole codebase and highly encourage the use of the event keyword for all usages of delegate callbacks.
Using the event keyword ensures that the delegate can only be invoked from within the class that declares it, maintaining proper encapsulation and control over the event's lifecycle and avoiding memory pressure from forgetting to unassign the delegates after use.

[NoelStephensUnity]

Other than:

• NetworkVariable.OnValueChanged (which you only want to subscribe to when it is spawned and always unsubscribe when despawned)
  • This is one area I would have to look through to determine how useful it would be vs the API change impacts
• NetworkTransform.OnClientRequestChange (which is a very legacy delegate callback, rarely used, and when used it is for very specific conditions)
• NetworkManager.ConnectionApprovalCallback (which is intentionally not an event)
• NetworkObject.OnMigratedToNewScene (could be converted to an event, but there would be an API impact)

Everything else is either an event or internal to the SDK or tests... but I don't see a whole bunch of the public API Actions/delegates that aren't events (quick pass are the 4 above) and if it is internal or part of a test then we are pretty good at cleaning up delegate callbacks

However, I do appreciate the insights you shared and I will definitely keep that pattern in mind.

Other than the 4 above listed, is this suggestion really more focused on making NetworkVariable.OnValueChanged to be an event or have you found the other 3 troublesome too? I just want to make sure I am not missing something else in the API that you are seeing.

[StephenHodgson]

NetworkVariable.OnValueChanged

I think mainly just this one is the most concerning since anyone can stomp delegate subscriptions with a simple typo.

[StephenHodgson]

NetworkManager.ConnectionApprovalCallback (which is intentionally not an event)

Why is that? Are you expecting people to set their own delegate for a callback? Better to use a Func<T> for that.

[NoelStephensUnity]

I think mainly just this one is the most concerning since anyone can stomp delegate subscriptions with a simple typo.

That is a good point and it will be something worth discussing as a potential improvement.

Why is that? Are you expecting people to set their own delegate for a callback? Better to use a Func for that.

Because Func<T> is a delegate that returns a value:

namespace System
{
    public delegate TResult Func<out TResult>();
}

And Action<T,T> is a delegate that accepts two parameters:

namespace System
{
    public delegate void Action<in T1, in T2>(T1 arg1, T2 arg2);
}

Where as the reason behind not making it an event is the implicit usage pattern of an Action vs an event Action where the former implies that you really should just use a single callback (i.e. direct assignment) as opposed to have several subscribers. Where just typing this makes me think more about trying to stick with this pattern and updating NetworkVariable.OnValueChanged to be public event OnValueChangedDelegate OnValueChanged as opposed to OnValueChangedDelegate OnValueChanged.