C#: Add WeakEvent to store subscriptions of events

[raulsntos]

• Add GodotWeakEvent type that stores the subscriptions of events.
  • Keeps only a weak reference to the target and the method info of the handler, so the target can be garbage collected.
  • If the target is collected, it will eventually get rid of the subscription data and avoids raising the event for collected targets.
    • Fixes Signal connections established with += are not automatically removed when receiver is destroyed #70414.
  • Used in user script events and native class events, resulting in consistent behavior.
    • Fixes C#: Error disconnecting from signal event when not connected in the first place #72994.
    • Supersedes C#: Safe -= from engine signal events #73062.
• Fix generation of signal events documentation that was adding the documentation comments in the wrong place.

[lorenzo-arena]

This should also fix #71032

[AThousandShips]

@juse4pro Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

This is still a draft so unsure when it'll be ready

[RedworkDE]

In general changing from strong events to weak events is a breaking change, eg obj.Event += () => Console.WriteLine("Event") may not get reliably get called when using weak events, while with strong events this will always trigger.

The weak bits could also be limited to just methods on GodotObject to somewhat limit the potential impact of this but then there would likely still be breaks and apparent leaks.

We could potentially make this opt-in/out-out on a per subscription basis to get the best of both, but that still requires care when using the signals.

[raulsntos]

In general changing from strong events to weak events is a breaking change

I think it should be fine to change the behavior here because it fixes a "bug". If you were previously making sure to disconnect from every signal then this shouldn't affect you, but if you weren't this should now do it for you.

Automatically disconnecting signals is the behavior many users have come to expect because that's how it works in GDScript and in C# with the Connect¹ method. This is one of the reasons many users are avoiding C# events and are still using Connect in 4.x², despite losing the type safety.

obj.Event += () => Console.WriteLine("Event") may not get reliably get called when using weak events, while with strong events this will always trigger.

As long as the delegate's target is still alive it should work the same way in both cases. If the target is not alive then removing the delegate from the subscriptions is the purpose of this PR, so users don't have to disconnect signals manually³. Maybe I misunderstood what you meant.

The weak bits could also be limited to just methods on GodotObject

I think it would be weird if methods on GodotObject get automatically unregistered but not others.

We could potentially make this opt-in/out-out on a per subscription basis to get the best of both

I'm not sure what you mean but one of the goals of this PR is to unify the event code for Godot native proxy-classes and user-defined classes. I'd want the same behavior for all signals, otherwise it'd be confusing.

Footnotes

1. The Connect method was the only way to connect signals in 3.x, so a lot of users are used to not having to manually disconnect from signals. ↩

2. We've been promoting C# events as the recommended way to use signals in C# with 4.x, so if users are avoiding them that's not great. ↩

3. As explained earlier, this is the behavior that users have come to expect. See Signal connections established with += are not automatically removed when receiver is destroyed #70414. ↩

[RedworkDE]

The problem that I was trying to (poorly) describe is that if the signal handler is a closure, the event will be the only reference to the closure object, so if that reference is made a weak reference, that object can now be collected and the event handler can no longer be called.

Example (that makes and actually can trigger this) binding additional arguments to a handler:

Timer timer;

void M1(int arg) {
   timer.Timeout += () => Handler(arg); // this will allocate a closure object that is only referenced by the signal
   timer.Start()
}

void Handler(int arg) {
   // previously this would be called forever after the signal is connected
   // afterwards this is only called until GC collected the closure
}

[raulsntos]

I see now, you are right when using closures they could be released and unsubscribed. Maybe documenting it could be enough.

The solution would be as simple as storing the closure object. We could provide a helper method for this:

Timer timer;

void M1(int arg) {
   timer.Timeout += KeepAlive(() => Handler(arg)); // KeepAlive will store the closure object so it's not released
   timer.Start();
}

void Handler(int arg) { }

But it would be trivial for the user to write their own.

static List<object> _keepAlives = new();

TEventHandler KeepAlive<TEventHandler>(TEventHandler handler) where TEventHandler : Delegate
{
	_keepAlives.Add(handler); // Store the closure object somewhere so it's not released
	return handler;
}

The bigger problem is that users would need to know that they need to do this, I can see how this could be easily missed. I guess it would possible to write an analyzer to check for this cases but it may not be easy to implement.

[BlankSourceCode]

I think that a lot of people are confused by the C# event behavior (I know I am) because of this comment in the docs page on C# Signals:

Note:
Godot will take care of disconnecting all the signals you connected through events when your nodes are freed. 
Meaning that: as you don't need to call Disconnect on all signals you used Connect on, 
you don't need to -= all the signals you used += on.

It sounds like information is currently incorrect, and you do actually need to call -= manually?

I think that it is going to be very common to use lambda expressions like += () => DoSomething() since that is a pretty typical pattern and is used in a lot of places in the docs. I would be hesitant to require extra syntax for keeping the reference alive as forgetting to use it would lead to additional confusion (as sometimes your handlers are just never called if they get GC'd).

I'm no expert in this area, but could we maybe make use of the auto generated backing_ field and query GetInvocationList() then check to see if the .Target of each handler is still alive (if freed call -= the handler). Perhaps in the RaiseGodotClassSignalCallbacks method just before the backing_ field Invoke() is called?

Even if that worked though, it still wouldn't be ideal because unregistering the handler wouldn't actually happen if you never emit the signal again, also I'm not sure on what the performance impact would be.

Just a thought.

[dashdotdashdot]

It sounds like information is currently incorrect, and you do actually need to call -= manually?

Yes, object instances that are disposed without having event subscriptions that call into them removed with -= presently throw ObjectDisposedException when those subscriptions try to fire. I solved a recent brush with this issue by removing them within an overridden Dispose method, though there are some risks in people having to do that as a first resort.

I think that it is going to be very common to use lambda expressions like += () => DoSomething() since that is a pretty typical pattern and is used in a lot of places in the docs.

I personally use this pattern all the time, both in the += () => DoSomething() form and to add context to parameterless signals in the style of += () => DoSomething(withCapturedVariable) (e.g. foo.TreeExiting += () => RemoveFromSomeSet(foo)) and I imagine I'd probably be very confused if such lambdas were to stop firing for what would seem like no apparent reason. Connected events being disappeared by the GC is quite a gotcha, even if documented. The GC also runs at different times from execution to execution, so it might end up being quite tricky for a developer to track down the snaggly problems this could create in games of any significant size or complexity.

I'm afraid I can't contribute much in the way of suggested solutions here. My two cents is that if I had to choose between auto-disconnection when using +=/-= and having connected lambdas be GC'd unless I knew to manually keep additional references or wrap them in a "please don't die" call, I'd rather what ends up producing the least surprising behaviour – no auto-disconnect in return for more obvious modes of operation and failure both, if that makes sense. 😃 On the other hand, being hit with ObjectDisposedExceptions that can only be avoided by manually -= the offending subscriptions at or before disposal is also painful – albeit more predictably so – so I hope it's a choice we end up not needing to make.