Signal connections established with `+=` are not automatically removed when receiver is destroyed

[derkork]

Godot version

v4.0.beta9.mono.official [e780dc3]

System information

Windows 10

Issue description

Assume you create a custom node class which emits a signal every second:

// Sender.cs
	[Signal]
	public delegate void MySignalEventHandler();
	
	public override void _Ready()
	{
		GetTree().CreateTimer(1).Timeout += OnTimeout;
	}

	
	private void OnTimeout()
	{
		EmitSignal(nameof(MySignal));
		GetTree().CreateTimer(1).Timeout += OnTimeout;
	}

Now you have a receiver which connects to this signal either with the classic "Connect" method or with the new += syntax. The receiver destroys itself after a few seconds:

// Receiver.cs
 public override void _Ready()
    {
        if (UseConnect)
        {
            Sender.Connect(nameof(Sender.MySignal), Callable.From(Receive));
        }
        else
        {
            Sender.MySignal += Receive;
        }
        
        // destroy after the specified time
        if (DestroyAfterSeconds > 0)
        {
            GetTree().CreateTimer(DestroyAfterSeconds).Timeout += QueueFree;
        }
    }

Either way, I would expect that the signal is automatically disconnected when when the receiver is destroyed. This works when using the Connect method, but doesn't work when using the += method. In the latter case the signal connection stays and the sender throws an exception when it emits the signal the next time:

E 0:00:10:0018   void Godot.Bridge.ScriptManagerBridge.RaiseEventSignal(IntPtr , Godot.NativeInterop.godot_string_name* , Godot.NativeInterop.godot_variant** , Int32 , Godot.NativeInterop.godot_bool* ): System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'sasake.Receiver'.
  <C++ Error>    Exception
  <C++ Source>   /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Object.base.cs:73 @ IntPtr Godot.Object.GetPtr(Godot.Object )()
  <Stack Trace>  Object.base.cs:73 @ IntPtr Godot.Object.GetPtr(Godot.Object )()
                 Node.cs:590 @ Godot.StringName Godot.Node.GetName()()
                 Node.cs:290 @ Godot.StringName Godot.Node.get_Name()()
                 Receiver.cs:31 @ void sasake.Receiver.Receive()()
                 Sender_ScriptSignals.generated.cs:26 @ void Sender.RaiseGodotClassSignalCallbacks(Godot.NativeInterop.godot_string_name& , Godot.NativeInterop.NativeVariantPtrArgs )()
                 ScriptManagerBridge.cs:341 @ void Godot.Bridge.ScriptManagerBridge.RaiseEventSignal(IntPtr , Godot.NativeInterop.godot_string_name* , Godot.NativeInterop.godot_variant** , Int32 , Godot.NativeInterop.godot_bool* )()

It looks like the sender tries to invoke a callback on the now disposed receiver. Again this works fine when using Connect to connect the signal.

Steps to reproduce

In the example project open the sender.tscn scene and run it. You can see in the output that the receiver receives two signals before it self destructs.

After that you can see the exception raised in the sender:

When you switch the receiver to use "Connect" using the inspector:

Then everything works as expected.

Minimal reproduction project

example.zip

[Calinou]

Is this operator overload for connecting signals documented anywhere? I don't remember seeing this in GDScript. Why was it added?

[derkork]

Is this operator overload for connecting signals documented anywhere? I don't remember seeing this in GDScript. Why was it added?

At least in the getting started section it is done with +=, e.g. timer.Timeout += OnTimerTimeout;.

[derkork]

It's probably caused by the generated implementation for the signal:

    private global::Sender.MySignalEventHandler backing_MySignal;
    /// <inheritdoc cref="global::Sender.MySignalEventHandler"/>
    public event global::Sender.MySignalEventHandler MySignal {
        add => backing_MySignal += value;
        remove => backing_MySignal -= value;
}

This wil just add the new event handler as a multicast target to the backing event handler, so the engine will not even know that the signal is connected. I checked that by printing the amount of signal connections in the sender. When using the += method, it reports 1 signal being connected (probably some internal connection) while when using the connect method it reports 2 signals being connected (the internal one and the one we added with Connect).

One solution could be to actually call "Connect" / Disconnect in the generated method and not use a "backing" signal delegate:

    public event global::Sender.MySignalEventHandler MySignal {
        add => Connect(nameof(MySignal), Callable.From(value));
        remove => Disconnect(nameof(MySignal), Callable.From(value)); // this will actually not work as it is a new callable
}

but you would need a way to transform the delegate type into a Callable plus you would need some kind of bookkeeping between the Callable and the original delegate that was used to create the callable so you can do a proper disconnect.

[markdibarry]

Is this operator overload for connecting signals documented anywhere? I don't remember seeing this in GDScript. Why was it added?

It is the current recommended way of connecting signals in C# and was introduced to replicate the built-in observer pattern in the C# language. You can see some examples here.

[lorenzo-arena]

Are there any news about this issue?

[Ark2000]

additional: it also throws System.ObjectDisposedException when the listener object is destroyed if you are using lambda or local function in Connect

like this:

Sender.Connect(nameof(Sender.MySignal), Callable.From(()=>{GD.Print("hello")}));

or

void sayHello()
{
    GD.Print("hello")
}
Sender.Connect(nameof(Sender.MySignal), Callable.From(sayHello));