[Core] Disconnect one-shot signals before calling callbacks

[AThousandShips]

Edit: Removed the recursion fix and can add it back in separately if desired, I still think it's an improvement but it makes this easier to merge and cherry-pick, keeping things as simple as possible for this case

This solves the issue, though I'd say some of the handling of await needs to be improved, see for example, so some further fixing of that side is likely needed:

• Code after the await keyword is executed in the main thread instead of the original thread. #79637

---

• Fixes: Errors when firing a signal just after awaiting on that signal #89439
• Fixes OneShot signal firing twice when retriggered in the signal's own callback #28857
• Fixes SignalAwaiter acts weirdly if same signal is emited synchronously #84254

[kleonc]

Removed the recursion fix

I'd say if anything it should be opened as a separate PR anyway, it seems a separate thing to the disconnecting change.

---

Also if this gets approved as us (just the disconnecting part) then I think we should be careful with cherry-picking it to 4.2, I'd say that's a change which is likely to introduce some regressions we haven't thought about.

---

I'm not sure if the change is safe. It's core-core so I'd say it needs to be assesed by reduz anyway, so let's wait for such assesment. 🙂

[BimDav]

Thanks for looking into this!

[reduz]

Left some notes on making the implementation more efficient

[AThousandShips]

Will update tomorrow, thank you!

[AThousandShips]

There, updated:

• Now using alloca instead of a LocalVector
• Not copying Connection as only the callable and flags parts are relevant, and the copy constructor for this type is actually very inefficient as it goes via a Dictionary to get there
• Removed the disconnect data entirely and just using the copied connections data

Still disconnecting all signals before emitting, to handle this:

signal my_signal

func foo():
    print("Foo")
    my_signal.emit()

func bar():
    print("Bar")

func _ready():
    my_signal.connect(foo, CONNECT_ONE_SHOT)
    my_signal.connect(bar, CONNECT_ONE_SHOT)
    my_signal.emit()

Without disconnecting all signals at once before emitting this will fire bar twice, as well as complain about disconnecting a non-existing signal

This cleans up a bunch of inefficiencies in this code, like storing the name of the signal in the disconnect data (not needed, only used in a single signal) and copying the whole Connection data, which involves copying a whole Signal

[AThousandShips]

memnew is required here to handle the uninitialized memory

[AThousandShips]

Gotta fix memory management here

Edit: Resolved, had to destroy them properly

[AThousandShips]

If there's a better way to handle the lifetime here do tell, I don't have an asan setup at the moment so can't test efficiently for leaks

[AThousandShips]

Added these for convenience, I ended up changing all the code below anyway as c went away but still neater

[AThousandShips]

Gonna add a GDScript test for this case and will push after CI has finished

[kleonc]

Gonna add a GDScript test for this case and will push after CI has finished

While at it probably good idea to also add some test specifically for the fixed await usage, something like in #89439 (comment).

[AThousandShips]

Will test out writing one!

[AThousandShips]

And done, that test will fail with errors on the unchanged branch, and behaves nicely now

[reduz]

Looks fantastic, great work!

[AThousandShips]

Thank you!

[akien-mga]

Thanks!

[AThousandShips]

Thank you!