Destroying an object running a coroutine (yield) should interrupt this coroutine properly

[Zylann]

Godot 3.0.6

This is one of the things that make me avoid yield often: basically, I like to use yield to lay down a series of asynchronous tasks in a normal function, which is very easy to follow and maintain. The only problem is, there is no clean way to interrupt such a task. By this, I mean I would expect the function to be stopped as if resuming it and immediately returning. So no "magic" cleaning, you would still have to put yield in proper places, but at least that would be something workable.

Currently, If the object running that coroutine gets destroyed (scene change, plugin disabled, enemy death, etc), it will throw the following error in the debugger:

ERROR: Resumed after yield, but class instance is gone
   At: modules/gdscript/gdscript_function.cpp:1490

YieldDestroyedObject.zip

I thought about putting an if stop after each yield, but that makes the code really ugly and still doesn't handle interruption properly. So I keep going back to _process to handle this, which is frustrating and time consuming.

Is that due to how yield was implemented? Is there a way to handle this?

[alwaysusa]

This is a frustrating issue. Yield is recommended in almost every tutorial I've read, without mention of the problem with not being able to interrupt it. If you free an object during a yield, it crashes. The various workarounds are complex and convoluted.

I'd love to see yields automatically handle object deletion. No one is assigned to this - so no chance this will be worked on?

[Calinou]

@alwaysusa GDScript is being rewritten for Godot 4.0, see #39093. yield will be replaced by an await keyword, though I don't know if the issue described here will be solved (it might be).

[alwaysusa]

@alwaysusa GDScript is being rewritten for Godot 4.0, see #39093. yield will be replaced by an await keyword, though I don't know if the issue described here will be solved (it might be).

Thanks for the info! I'm guessing 4.0 is a ways off. I'll have to break up my current use of yields into timers and signals instead.

Thanks again :)

[jamie-pate]

more discussion here:
https://godotengine.org/qa/8656/how-properly-stop-yield-from-resuming-after-the-class-freed?show=86173#a86173

[jkb0o]

Actually, you can keep use yields in this case, this is just a warning. If the instance is gone after resuming yield, there is just no code that will be executed after. I use yields a lot, but I have to recompile the engine with this warning disabled as it seems useless.

[jamie-pate]

Agree, the developers say 'be sure to eliminate all errors during production' but there's a bunch of stuff like this that prints out that you can't easily eliminate

[Paspartout]

As long as you use yield only on builtin nodes without nested coroutines this is indeed fine, but once free an object/node that has running nested coroutines it seems to corrupt memory: #47703

I think these issues are related so I bring it up here. I really would love to use coroutines more often, they are such an elegant way of expressing sequences of actions.

[jkb0o]

As long as you use yield only on builtin nodes without nested coroutines this is indeed fine, but once free an object/node that has running nested coroutines it seems to corrupt memory: #47703

I think these issues are related so I bring it up here. I really would love to use coroutines more often, they are such an elegant way of expressing sequences of actions.

We use coroutines a lot. I mean A LOT. And there are zero problems with them. The issue you mentioned is generally about asan compilation flag, and based on your stack trace, I doubt asan's confidence about clearing Reference owned by RefPtr twice.

[Paspartout]

Well in my latest jam project I also used coroutines alot(baiscally every animation and enemy) and had the game crashing seemingly random and fixed it by making sure each coroutines is stopped before destroying the owning object. I think when running the mentioned code in #47703 long enough without asan it would crash in the same way. Might give it a try. Ignoring asan errors is not a good Idea I think.

I used timer nodes though instead of get_tree().create_timer(t) to make the error mentioned in this issue go away, that's probably what you are using and not having probelms with. Indeed when changing the Code in #47703 to this, I get no asan warning, just the godot error mentioned in this issue.

extends Node

func _ready():
	coro()
	queue_free()
	print("No Crash/Memory corruption")

func coro():
	while true:
		yield(get_tree().create_timer(1.0), "timeout")
		print("Please don't free me")

I just mentioned my issue here because I think solving this issue should also solve mine and vice versa. They are related but not the same.

[sttifer]

I have the same problem. My question is, when an instance is "deleted", the coroutines is still stored in memory waiting?
Example: An enemy has a state and is waiting for the state of the previous one to finish before executing this new state. But before going to the next one the enemy is dead. What happens? Does the coroutine remain in memory waiting for the previous state forever?

[tavurth]

Had to spend hours today rewriting my existing code which was heavily coroutine based because of this issue.

I agree that it's frustrating because a lot of the documentation and helpful tips show use of yield.

This issue also seems to present more when the CPU has more load due to race conditions. This means that while Godot may be fast for prototyping, later in the project you are pretty much guaranteed to have a bad time if you use yield a lot.

I think since this seems unlikely to be fixed, we should get the word out that yield is basically bad GDScript form and that you should avoid it at all costs, especially in larger projects.

[samsface]

This kinda pattern were you tie the coroutine to the owning node should solves most issues.
Here is a safe version of yield(get_tree(), "idle_frame")

extends Reference
class_name TempTimer

class FrameTimer_ extends Node:
	signal timeout

	var frames := 1

	func _ready() -> void:
		get_tree().connect("idle_frame", self, "_on_timeout")

	func _on_timeout() -> void:
		frames -= 1
		if frames == 0:
			emit_signal("timeout")
			queue_free()

static func idle_frame(node:Node, frames:int = 1):
	var t   := FrameTimer_.new()
	t.frames = frames
	node.add_child(t)
	
	return t

Useage:

func some_func():
   yield(TempTimer.idle_frame(self), "timeout")

Though I agree with the sentiment of this issue, just giving a work around for people.

[and3rson]

This is a very nasty caveat considering how many times it's mentioned in the docs as a best practice.

I think resumes on freed objects should behave more intuitively. The developer would typically expect from a coroutine whose owner was freed to stop, similarly to how a node stops being processed when it's removed from a tree or freed.

In general, yields are awesome for keeping complex sequences of steps. I find myself using this pattern a lot:

$shoot.play()
$animation_player.play('shoot')
var recoil = rand_range(-0.25, -1)
yield(get_tree().create_timer(0.25), 'timeout')
$reload.play("recoil", -1, 1 / recoil)
yield(get_tree().create_timer(0.15 * reload_speed), 'timeout')
$reload2.play();
yield(get_tree().create_timer(0.15 * recoil), 'timeout')
# etc etc

However once the node is freed (e. g. user has switched weapons), problems arise.

One can argue that I could use AnimationPlayer here, but:

• With yields, everything is kept within a single block of code in a single function.
• Timeouts and branches can be defined in runtime, whereas AnimationPlayer only allows a strict sequence of steps.

[Calinou]

3.5 has the new Tween system backported as SceneTreeTween, so you may no longer have to use yield() in this scenario.

Last time I heard, yield() is unlikely to be fixed as its design is fundamentally flawed. However, yield() has been removed entirely in favor of await in 4.0.alpha1 and later, so this is indirectly fixed in 4.0.

[vpellen]

this is indirectly fixed in 4.0.

Afraid not. Issue persists with await in Alpha 12 Beta 17.

extends Node

func _ready():
	queue_free()
	await get_tree().create_timer(1).timeout

E 0:00:02:0183   resume: Resumed function '_ready()' after await, but script is gone. At script: res://testbed.tscn::GDScript_yotee:5
  <C++ Error>    Method/function failed. Returning: Variant()
  <C++ Source>   modules/gdscript/gdscript_function.cpp:226 @ resume()

[hsandt]

@samsface 's answer inspired me to write this util, using Godot native Timer this time:

EDIT: check out the latest version more below: #24311 (comment)

extends Reference
class_name TempTimer

static func create_timer_under(node: Node, duration: float):
	var temp_timer = Timer.new()
	node.add_child(temp_timer)
	temp_timer.start(duration)
	return temp_timer
	
# Usage
# Instead of:
#	yield(get_tree().create_timer(seconds), "timeout")
# write:
#	yield(TempTimer.create_timer_under(node, seconds), "timeout")
# where node is some node with lifetime in sync with when we want to keep running the coroutine
# (typically the object affected by coroutine, or an object with same lifetime)

I tested it and it also fixed the issue. In my case the caller self had the same lifetime as the object manipulated by the coroutine, so I just passed node = self.

[jamie-pate]

@hsandt unfortunately your fix will leak timers as you have no free call. Maybe you could create a timer subclass that is always oneshot and auto queue frees itself?

[darthdeus]

samsface 's answer inspired me to write this util, using Godot native Timer this time:

@hsandt Is this fix actually needed since #65910 apparently fixes the issue?

[jamie-pate]

Afaik this issue is not back ported to 3.x, so a workaround may be needed there for now

[hsandt]

@hsandt unfortunately your fix will leak timers as you have no free call. Maybe you could create a timer subclass that is always oneshot and auto queue frees itself?

OK, I got a new version (with a weird workaround because of another bug - #21461 (comment))

# Adapted from https://github.com/godotengine/godot/issues/24311#issuecomment-1059531870
extends Timer
class_name TempTimer

func _ready() -> void:
	# Temporary timers are always one-shot
	one_shot = true
	
	# Free itself on timeout, if parent node was not already freed
	var _err = connect("timeout", self, "_on_timeout")

func _on_timeout() -> void:
	queue_free()

static func create_temp_timer_under(node: Node, duration: float):
	# The 2 lines below are equivalent to `var temp_timer = TempTimer.new()`
	# but we need this workaround to avoid:
	# > Parser Error: Using own name in class file is not allowed (creates a cyclic reference)
	# See https://github.com/godotengine/godot/issues/21461#issuecomment-466565674
	# We could also move this static function in a different script
	var Self = load("res://Scripts/Utils/TempTimer.gd")
	var temp_timer = Self.new()
	node.add_child(temp_timer)
	temp_timer.start(duration)
	return temp_timer

# USAGE
#
# Instead of:
#	yield(get_tree().create_timer(seconds), "timeout")
# write:
#	yield(TempTimer.create_temp_timer_under(node, seconds), "timeout")
# where node is some node with lifetime in sync with when we want to keep running the coroutine
# (typically the object affected by coroutine, or an object with same lifetime)

[hsandt]

And a two files version to avoid the hacky workaround:

# TempTimer.gd

# Alternative to Timer to use in contexts where the Timer is used by a coroutine
# that works on certain nodes, but the nodes may be freed in the middle of the coroutine
# (due to game restart, etc.)
# 
# This is a temporary timer that you should place with static function
# `TimerUtils.create_temp_timer_under` so it is placed under some representative node
# affected by the coroutine, and freed when the node is freed, thus avoiding
# an error due to coroutine trying to access the freed node:
# > resume: Resumed function 'my_coroutine()' after yield, but class instance is gone
#
# This simulates what SceneTreeTimer does (auto-free on timeout) while having
# a proper timer node placed under a parent so it gets freed with it.
#
# Adapted from https://github.com/godotengine/godot/issues/24311#issuecomment-1059531870
#
# USAGE
# See TimerUtils.gd
extends Timer
class_name TempTimer

func _ready() -> void:
	# Temporary timers are always one-shot
	one_shot = true
	
	# Free itself on timeout, if parent node was not already freed
	var _err = connect("timeout", self, "_on_timeout")

func _on_timeout() -> void:
	queue_free()

# TimerUtils.gd

class_name TimerUtils

# This function creates a TempTimer under a node so it supports node deletion
# during a coroutine (see TempTimer.gd).
#
# USAGE
# Instead of:
#	yield(get_tree().create_timer(seconds), "timeout")
# write:
#	yield(TimerUtils.create_temp_timer_under(node, seconds), "timeout")
# where node is some node with lifetime in sync with when we want to keep running the coroutine
# (typically the object affected by coroutine, or an object with same lifetime)

static func create_temp_timer_under(node: Node, duration: float):
	var temp_timer = TempTimer.new()
	node.add_child(temp_timer)
	temp_timer.start(duration)
	return temp_timer