godot: Errors when firing a signal just after awaiting on that signal

The direct cause is that ONE_SHOT connections to a signal are disconnected after the connected method is called, not before. Meaning e.g. this results in an infinite loop:

extends Node2D

signal my_signal

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

func foo():
	my_signal.emit()

Related issue: #28857.

How does it affect await? When awaiting on a signal GDScriptFunctionState::_signal_callback which calls GDScriptFunctionState::resume is ONE_SHOT-connected to such awaited signal:

https://github.com/godotengine/godot/blob/61282068f4d59cb48f35ad95391728c58d9008ab/modules/gdscript/gdscript_vm.cpp#L2306 https://github.com/godotengine/godot/blob/61282068f4d59cb48f35ad95391728c58d9008ab/modules/gdscript/gdscript_function.cpp#L137-L167

Meaning if the awaited signal is emitted again during such resume() then it results in calling resume() again because the ONE_SHOT connection is not yet disconnected.

extends Node2D

signal my_signal

func _ready() -> void:
	foo()
	my_signal.emit() # A

func foo():
	await my_signal
	my_signal.emit() # B

Execution goes something like:

`_ready()`
	`foo()`
		`await my_signal`
			`ONE_SHOT` connect resuming `foo` on `my_signal`
	`my_signal.emit() # A`
		Resume `foo`.
			`my_signal.emit() # B`
				Resume `foo`.
					Error (already removed from `scripts_list` within the first resume).
				Disconnect resuming `foo` on `my_signal` signal (it's ONE_SHOT and `my_signal.emit() # B` handled all connections).
			foo "completed" signal
		Error trying to disconnect resuming `foo` on `my_signal` signal (`my_signal.emit() # A` handled all connections).

The in GDScriptFunctionState::resume error part: https://github.com/godotengine/godot/blob/61282068f4d59cb48f35ad95391728c58d9008ab/modules/gdscript/gdscript_function.cpp#L190-L212

How disconnecting ONE_SHOT connections is done: https://github.com/godotengine/godot/blob/61282068f4d59cb48f35ad95391728c58d9008ab/core/object/object.cpp#L1121-L1175

enforcing protection against recursion in signals

I’m absolutely against this, there’s nothing wrong with the recursive signals on its own. Sure an infinite recursion is unwanted/problematic but if it happens then that would be a user error (assuming no in-engine bugs). Just because recursion can be misued doesn’t mean it should be disallowed.

BTW it’s already allowed hence changing it I’d consider as a breaking change (as I don’t think it’s a bug / needs a bugfix).

And that’s also kinda a separate discussion, even possibly a material for a proposal.

It’s just a minimal code example to reproduce the bug.

Just for information, you can call an async function without waiting for it. So the following example is simpler since it doesn’t involve node processing:

extends Node

signal my_signal()

func _ready():
    async_func()
    my_signal.emit()

func async_func():
    await my_signal
    my_signal.emit()

@AThousandShips To clarify: I’ve only pointed out the direct cause of the reported issue for the current implementation.

Whether how ONE_SHOT connections are being disconnected is intentional or it is a bug itself I don’t know, hence why I’ve asked about this in the contributors chat (I could have mentioned my doubts in my previous comment).

But regardless of the current ONE_SHOT disconnecting behavior, what’s reported here i.e. await producing errors was already triaged as a bug by a GDScript team member (dalexeev) so please respect this more / be more thoughful about changing the labels in the future (I’ll leave fixing them to you).

The behavior of await seems already correct / as expected, what’s reported here is some superfluous/unneeded errors.

So:

• If the current ONE_SHOT disconnecting behavior is a bug then it needs to be fixed. The issue with await erroring would then be automatically fixed as a result.
• If the current ONE_SHOT disconnecting behavior is not a bug then await’s implementation needs to be tweaked to fix the reported issue accordingly.

@BimDav Your original MRP is fine/sufficient on its own, no need for a “real world” example to classify some easily reproducible behavior as a bug / not a bug.

@kleonc Thanks for investigating! @AThousandShips it is still a bug, though, right? Why did you remove the bug tag?

It’s just a minimal code example to reproduce the bug. The bug is not linked to this happening in _ready or not. The bug is also not linked to the set_process(false), which I added to remove confusion due to the signal being emitted each frame. This way, it is only emitted once.