Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested coroutines crash the engine after freeing the executing node #47703

Closed
Paspartout opened this issue Apr 7, 2021 · 3 comments
Closed

Comments

@Paspartout
Copy link

Godot version: 3.2.3(tag) and 3.3rc7(hash mentioned in news post), both built using asan=True

OS/device including version:
Linux x64 for reproduction, but issue happened on Windows and WASM/Web build too

Issue description:

When running the minimal example project without the address sanitizer everything seems fine at first at first, but eventually the engine would crash. That's what happened in a bigger project of mine. After investigation I found out that it is caused by waiting on a coroutine to complete while also trying to free the corresponding node(see example below).

Steps to reproduce:

  1. Build one of the mentioned godot versions with asan=true.
  2. Run the minimal example project
  3. Crash with following log: https://gist.github.com/Paspartout/3411dd1f0a9c49161f49ca99f7df03cb

Minimal reproduction project:

The following script attached to any node will trigger the crash/memory corruption:

extends Node

var timer: Timer

func _ready():
	timer = Timer.new()
	add_child(timer)
	timer.start()
	coro() # switching this to coro_ok() fixes the crash
	queue_free()
	print("Incoming Crash/Memory corruption")

func wait(time: float):
	timer.wait_time = time
	timer.start()
	yield(timer, "timeout")

func coro():
	while true:
		yield(wait(1.0), "completed")
		print("Please don't free me")

func coro_ok():
	while true:
		timer.wait_time = 1.0
		timer.start()
		yield(timer, "timeout")
		print("You can free me!")

coro_crash.zip

@Calinou Calinou added this to the 3.4 milestone Apr 7, 2021
Paspartout added a commit to Paspartout/MoonDefense that referenced this issue Apr 7, 2021
This makes sure all coroutines ended before calling queue_free() which
currently causes memory problems.
See godotengine/godot#47703
Paspartout added a commit to Paspartout/MoonDefense that referenced this issue Apr 9, 2021
This makes sure all coroutines ended before calling queue_free() which
currently causes memory problems.
See godotengine/godot#47703
@HybridEidolon
Copy link
Contributor

HybridEidolon commented Oct 30, 2021

I ported the given script to GDScript 2.0 and ran it in v4.0.dev.20211015.official [f113dc9]

extends Node

var timer: Timer

func _ready():
    timer = Timer.new()
    add_child(timer)
    timer.start()
    coro() # switching this to coro_ok() fixes the crash
    queue_free()
    print("Incoming Crash/Memory corruption")

func wait(time: float):
    timer.wait_time = time
    timer.start()
    await timer.timeout

func coro():
    while true:
        await wait(1.0)
        print("Please don't free me")

func coro_ok():
    while true:
        timer.wait_time = 1.0
        timer.start()
        await timer.timeout
        print("You can free me!")

Neither coro() nor coro_ok() will crash the engine when used.

On v3.4.rc2.official.23955fc28, it is also fixed, somehow.

Edit: oh, I should mention I ran them presumably without asan. So they may still be present, but it doesn't appear to affect the official builds here.

@akien-mga akien-mga modified the milestones: 3.4, 3.5 Nov 8, 2021
@KoBeWi KoBeWi modified the milestones: 3.5, 3.x Apr 23, 2024
@Capital-EX
Copy link

I was talking about concurrency in Godot with a small discord group. And, the issue around freeing an objects which have pending coroutines came up. I can confirm this issue forced me to upgrade a project from Godot 3.0 to Godot 4.0 (where the issue has been fixed for await #24311). Additionally, it resulted in me abandoning a coroutine-based library and caused a major rewrite to a sizable project.

This bug is highly non-deterministic, but will always fail when ran with an address sanitizer (see #74695).

(possibly related to #71998 and #74695)

@lawnjelly
Copy link
Member

Closed by #97464.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants