godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.34k stars 20.01k forks source link

freeing `PhysicsServer3DExtension` crashs without any info #67643

Open MikeSchulze opened 1 year ago

MikeSchulze commented 1 year ago

Godot version

v4.0.beta3.official [01ae26d31]

System information

windows 10

Issue description

I run in this issue while migration my unit test api. To test extraction of class names from engine classes i loop over all engine classes provided by ClassDB.get_class_list() My test aborts without any reason, no error in the console

After adding more log infos i find out freeing a instance of PhysicsServer3DExtension aborts function calls without any error.

Steps to reproduce

Run the code and you see the line prints("freed") is not executed.

func _ready():
    var instance := PhysicsServer3DExtension.new()
    prints(instance, instance.has_method("free"))
    # await get_tree().process_frame
    instance.free()
    prints("freed")
    await get_tree().process_frame

To force to see the "real" error i try to add an extra await process_frame Rerun the code and comment in the line # await get_tree().process_frame Now an error is shown image

You can run over all engine classes and see the debuger crashes randomly. The line prints("--- Done ---") is never called

func _ready():
    for clazz_name in ClassDB.get_class_list():
        if not ClassDB.can_instantiate(clazz_name):
            continue
        var instance = ClassDB.instantiate(clazz_name)
        prints(clazz_name, instance)
        await get_tree().process_frame
        if not instance is RefCounted:
            prints("---> free()", instance, instance.has_method("free"))
            instance.free()
            prints("freed")
            await get_tree().process_frame
    prints("--- Done ---")

Minimal reproduction project

No response

rburing commented 1 year ago

This happens because the destructor sets the PhysicsServer3D singleton to nullptr.

To avoid this we could take the approach used by TextServer, which is to make the servers RefCounted:

https://github.com/godotengine/godot/blob/040f49ed6e71a6e7f23d763c4b56095cbf319ef7/servers/text_server.h#L46

and let the server manager (singleton) keep a reference to the primary implementation:

https://github.com/godotengine/godot/blob/040f49ed6e71a6e7f23d763c4b56095cbf319ef7/servers/text_server.h#L548

This would fix the issue, though I don't think it's very high priority, and we would have to find a good replacement for the ~400 occurrences of PhysicsServer3D::get_singleton() in the existing code base. I'm not a big fan of using an acronym like

https://github.com/godotengine/godot/blob/040f49ed6e71a6e7f23d763c4b56095cbf319ef7/servers/text_server.h#L574

instead of something more explicit. Maybe PhysicsServer3D::get_singleton() could remain and we make it call the manager instead, which would be a simple solution though it feels slightly dirty. cc @bruvzg for thoughts on the design/style.