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

[derkork]

Godot version

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

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

[fabianPas]

Any news on this problem? I'm experiencing it as well and think it's quite a nasty one as the documentation is filled with event connecting examples instead of .Connect().

[AThousandShips]

See the linked PR:

• 73730

[fabianPas]

I see, thank you very much!

[hunterloftis]

This issue is over a year old. Given the nasty set of bugs it silently produces, should it be either:

1. Prioritized for a fix or
2. Prioritized for correct documentation (replacing all examples of += with a safer mechanism?)

At the very least, it would be useful to provide a non-+= alternative in the documentation about passing arguments with signals, which currently only lists the leaky version:

https://docs.godotengine.org/en/stable/tutorials/scripting/c_sharp/c_sharp_signals.html#custom-signals-as-c-events

[31]

About a month ago, I contributed some PRs to the C# signals doc to describe the behavior with some more detail. However, it's only available in latest (4.3): https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/c_sharp_signals.html. It includes a general example for how to use += with closures in a way that keeps the delegate around to pass to -= in a cleanup method.

PTAL and send some followup PRs if you can make it clearer! I've gotten feedback that the example is a bit too contrived and makes it hard to follow the rest of it, but I haven't had a chance to go back and see if I can make it better by giving more context or using a more gamedev-linked example.

[hunterloftis]

@31 thanks for the link, your additions are a huge improvement to the docs in that section! I especially appreciate the detailed info about what's happening and when using Connect will and won't help.

[tm76]

Appreciate the work that has gone into the updated documentation for 4.3, and the information collected to this point.

Took a fair bit of digging to find out what my issue was with an ObjectDisposedException, and the += now am aware that the feature parity for C# for signals doesn't actually have the automated disconnection.

Seems like this is a reasonable priority to address in a future release, not just updating the documentation with the workarounds.

[hunterloftis]

Agreed @tm76, given that signals are such a core part of Godot, most C# users would likely be surprised that they aren't rock solid / have so many gotchas.

[HymnStudio]

About a month ago, I contributed some PRs to the C# signals doc to describe the behavior with some more detail. However, it's only available in latest (4.3): https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/c_sharp_signals.html. It includes a general example for how to use += with closures in a way that keeps the delegate around to pass to -= in a cleanup method.

PTAL and send some followup PRs if you can make it clearer! I've gotten feedback that the example is a bit too contrived and makes it hard to follow the rest of it, but I haven't had a chance to go back and see if I can make it better by giving more context or using a more gamedev-linked example.

As you said in the latest doc that using Connect will disconnect automatically with custom signals, while using += needs manual disconnection, I wonder whether connecting through Godot Editor Inspector equals using Connect method？Thank you！

[dante-signal31]

@HymnStudio I think editor signal connections indeed equals those using Connect().

The test I've done:

1. I've put a QueueFree() in a receiver object so it is removed after first signal emission.
2. I emitted once the signal and receiver has been properly removed.
3. In the second signal emission debugger does not throw any error. So I guess signal receiver was properly disconnected from emissor when QueueFree() was called.