Unity-Technologies / Unity-Technologies/com.unity.netcode.gameobjects

Naked delegates should have event modifier keyword

Đang mở
#3,137 4 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

type:feedback
Ngôn ngữ chính
C#
Star
2.3k
Fork
461
Merge trung bình
3 ngày 16 giờ
Pull request đã merge (30 ngày)
20

Mô tả

https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/blob/dcaeef9ab7271c8e25081b484f9cb0be34294ecd/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs#L23

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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu với Runtime/NetworkVariable/NetworkVariable.cs tại dòng 23, sau đó sử dụng tính năng tìm kiếm mã của repository được liên kết để tìm các khai báo public delegate callback trên toàn bộ codebase. Kiểm tra từng delegate chưa được đóng gói và thêm modifier event khi phù hợp; hoàn tất khi các delegate callback đã xác định được đóng gói nhất quán dưới dạng events.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
csharp, unity
Lĩnh vực
networking
Loại issue
Tái cấu trúc
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Đình trệ
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
35/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.