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

Clean up instance bindings for engine singletons to prevent crash #1458

Merged
merged 1 commit into from
May 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion binding_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,10 @@ def generate_engine_class_header(class_api, used_classes, fully_used_classes, us
result.append(f"\tGDEXTENSION_CLASS({class_name}, {inherits})")
result.append("")

if is_singleton:
result.append(f"\tstatic {class_name} *singleton;")
result.append("")

result.append("public:")
result.append("")

Expand Down Expand Up @@ -1586,6 +1590,11 @@ def generate_engine_class_header(class_api, used_classes, fully_used_classes, us

result.append("\t}")
result.append("")

if is_singleton:
result.append(f"\t~{class_name}();")
result.append("")

result.append("public:")

# Special cases.
Expand Down Expand Up @@ -1735,6 +1744,7 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us

result.append(f"#include <godot_cpp/classes/{snake_class_name}.hpp>")
result.append("")
result.append("#include <godot_cpp/core/class_db.hpp>")
result.append("#include <godot_cpp/core/engine_ptrcall.hpp>")
result.append("#include <godot_cpp/core/error_macros.hpp>")
result.append("")
Expand All @@ -1749,9 +1759,10 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us
result.append("")

if is_singleton:
result.append(f"{class_name} *{class_name}::singleton = nullptr;")
result.append("")
result.append(f"{class_name} *{class_name}::get_singleton() {{")
# We assume multi-threaded access is OK because each assignment will assign the same value every time
result.append(f"\tstatic {class_name} *singleton = nullptr;")
result.append("\tif (unlikely(singleton == nullptr)) {")
result.append(
f"\t\tGDExtensionObjectPtr singleton_obj = internal::gdextension_interface_global_get_singleton({class_name}::get_class_static()._native_ptr());"
Expand All @@ -1765,11 +1776,22 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us
result.append("#ifdef DEBUG_ENABLED")
result.append("\t\tERR_FAIL_NULL_V(singleton, nullptr);")
result.append("#endif // DEBUG_ENABLED")
result.append("\t\tif (likely(singleton)) {")
result.append(f"\t\t\tClassDB::_register_engine_singleton({class_name}::get_class_static(), singleton);")
result.append("\t\t}")
result.append("\t}")
result.append("\treturn singleton;")
result.append("}")
result.append("")

result.append(f"{class_name}::~{class_name}() {{")
result.append("\tif (singleton == this) {")
result.append(f"\t\tClassDB::_unregister_engine_singleton({class_name}::get_class_static());")
result.append("\t\tsingleton = nullptr;")
result.append("\t}")
result.append("}")
result.append("")

if "methods" in class_api:
for method in class_api["methods"]:
if method["is_virtual"]:
Expand Down
18 changes: 18 additions & 0 deletions include/godot_cpp/core/class_db.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include <godot_cpp/variant/callable_method_pointer.hpp>

#include <list>
#include <mutex>
#include <set>
#include <string>
#include <unordered_map>
Expand Down Expand Up @@ -104,6 +105,8 @@ class ClassDB {
static std::unordered_map<StringName, const GDExtensionInstanceBindingCallbacks *> instance_binding_callbacks;
// Used to remember the custom class registration order.
static std::vector<StringName> class_register_order;
static std::unordered_map<StringName, Object *> engine_singletons;
static std::mutex engine_singletons_mutex;

static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const MethodDefinition &method_name, const void **p_defs, int p_defcount);
static void initialize_class(const ClassInfo &cl);
Expand Down Expand Up @@ -153,6 +156,21 @@ class ClassDB {
instance_binding_callbacks[p_name] = p_callbacks;
}

static void _register_engine_singleton(const StringName &p_class_name, Object *p_singleton) {
std::lock_guard<std::mutex> lock(engine_singletons_mutex);
std::unordered_map<StringName, Object *>::const_iterator i = engine_singletons.find(p_class_name);
if (i != engine_singletons.end()) {
ERR_FAIL_COND((*i).second != p_singleton);
return;
}
engine_singletons[p_class_name] = p_singleton;
}

static void _unregister_engine_singleton(const StringName &p_class_name) {
std::lock_guard<std::mutex> lock(engine_singletons_mutex);
engine_singletons.erase(p_class_name);
}

template <typename N, typename M, typename... VarArgs>
static MethodBind *bind_method(N p_method_name, M p_method, VarArgs... p_args);

Expand Down
1 change: 1 addition & 0 deletions include/godot_cpp/godot.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ extern "C" GDExtensionInterfaceObjectDestroy gdextension_interface_object_destro
extern "C" GDExtensionInterfaceGlobalGetSingleton gdextension_interface_global_get_singleton;
extern "C" GDExtensionInterfaceObjectGetInstanceBinding gdextension_interface_object_get_instance_binding;
extern "C" GDExtensionInterfaceObjectSetInstanceBinding gdextension_interface_object_set_instance_binding;
extern "C" GDExtensionInterfaceObjectFreeInstanceBinding gdextension_interface_object_free_instance_binding;
extern "C" GDExtensionInterfaceObjectSetInstance gdextension_interface_object_set_instance;
extern "C" GDExtensionInterfaceObjectGetClassName gdextension_interface_object_get_class_name;
extern "C" GDExtensionInterfaceObjectCastTo gdextension_interface_object_cast_to;
Expand Down
18 changes: 18 additions & 0 deletions src/core/class_db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ namespace godot {
std::unordered_map<StringName, ClassDB::ClassInfo> ClassDB::classes;
std::unordered_map<StringName, const GDExtensionInstanceBindingCallbacks *> ClassDB::instance_binding_callbacks;
std::vector<StringName> ClassDB::class_register_order;
std::unordered_map<StringName, Object *> ClassDB::engine_singletons;
std::mutex ClassDB::engine_singletons_mutex;
GDExtensionInitializationLevel ClassDB::current_level = GDEXTENSION_INITIALIZATION_CORE;

MethodDefinition D_METHOD(StringName p_name) {
Expand Down Expand Up @@ -419,6 +421,22 @@ void ClassDB::deinitialize(GDExtensionInitializationLevel p_level) {
});
class_register_order.erase(it, class_register_order.end());
}

if (p_level == GDEXTENSION_INITIALIZATION_CORE) {
// Make a new list of the singleton objects, since freeing the instance bindings will lead to
// elements getting removed from engine_singletons.
std::vector<Object *> singleton_objects;
{
std::lock_guard<std::mutex> lock(engine_singletons_mutex);
singleton_objects.reserve(engine_singletons.size());
for (const std::pair<StringName, Object *> &pair : engine_singletons) {
singleton_objects.push_back(pair.second);
}
}
for (std::vector<Object *>::iterator i = singleton_objects.begin(); i != singleton_objects.end(); i++) {
internal::gdextension_interface_object_free_instance_binding((*i)->_owner, internal::token);
}
}
}

} // namespace godot
2 changes: 2 additions & 0 deletions src/godot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ GDExtensionInterfaceObjectDestroy gdextension_interface_object_destroy = nullptr
GDExtensionInterfaceGlobalGetSingleton gdextension_interface_global_get_singleton = nullptr;
GDExtensionInterfaceObjectGetInstanceBinding gdextension_interface_object_get_instance_binding = nullptr;
GDExtensionInterfaceObjectSetInstanceBinding gdextension_interface_object_set_instance_binding = nullptr;
GDExtensionInterfaceObjectFreeInstanceBinding gdextension_interface_object_free_instance_binding = nullptr;
GDExtensionInterfaceObjectSetInstance gdextension_interface_object_set_instance = nullptr;
GDExtensionInterfaceObjectGetClassName gdextension_interface_object_get_class_name = nullptr;
GDExtensionInterfaceObjectCastTo gdextension_interface_object_cast_to = nullptr;
Expand Down Expand Up @@ -442,6 +443,7 @@ GDExtensionBool GDExtensionBinding::init(GDExtensionInterfaceGetProcAddress p_ge
LOAD_PROC_ADDRESS(global_get_singleton, GDExtensionInterfaceGlobalGetSingleton);
LOAD_PROC_ADDRESS(object_get_instance_binding, GDExtensionInterfaceObjectGetInstanceBinding);
LOAD_PROC_ADDRESS(object_set_instance_binding, GDExtensionInterfaceObjectSetInstanceBinding);
LOAD_PROC_ADDRESS(object_free_instance_binding, GDExtensionInterfaceObjectFreeInstanceBinding);
LOAD_PROC_ADDRESS(object_set_instance, GDExtensionInterfaceObjectSetInstance);
LOAD_PROC_ADDRESS(object_get_class_name, GDExtensionInterfaceObjectGetClassName);
LOAD_PROC_ADDRESS(object_cast_to, GDExtensionInterfaceObjectCastTo);
Expand Down
3 changes: 3 additions & 0 deletions test/project/main.gd
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ func _ready():
assert_equal(example.test_virtual_implemented_in_script("Virtual", 939), "Implemented")
assert_equal(custom_signal_emitted, ["Virtual", 939])

# Test that we can access an engine singleton.
assert_equal(example.test_use_engine_singleton(), OS.get_name())

# Test that notifications happen on both parent and child classes.
var example_child = $ExampleChild
assert_equal(example_child.get_value1(), 11)
Expand Down
7 changes: 7 additions & 0 deletions test/src/example.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <godot_cpp/classes/label.hpp>
#include <godot_cpp/classes/multiplayer_api.hpp>
#include <godot_cpp/classes/multiplayer_peer.hpp>
#include <godot_cpp/classes/os.hpp>
#include <godot_cpp/variant/utility_functions.hpp>

using namespace godot;
Expand Down Expand Up @@ -239,6 +240,8 @@ void Example::_bind_methods() {
GDVIRTUAL_BIND(_do_something_virtual, "name", "value");
ClassDB::bind_method(D_METHOD("test_virtual_implemented_in_script"), &Example::test_virtual_implemented_in_script);

ClassDB::bind_method(D_METHOD("test_use_engine_singleton"), &Example::test_use_engine_singleton);

ClassDB::bind_static_method("Example", D_METHOD("test_static", "a", "b"), &Example::test_static);
ClassDB::bind_static_method("Example", D_METHOD("test_static2"), &Example::test_static2);

Expand Down Expand Up @@ -671,6 +674,10 @@ String Example::test_virtual_implemented_in_script(const String &p_name, int p_v
return "Unimplemented";
}

String Example::test_use_engine_singleton() const {
return OS::get_singleton()->get_name();
}

void ExampleRuntime::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_prop_value", "value"), &ExampleRuntime::set_prop_value);
ClassDB::bind_method(D_METHOD("get_prop_value"), &ExampleRuntime::get_prop_value);
Expand Down
2 changes: 2 additions & 0 deletions test/src/example.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ class Example : public Control {

GDVIRTUAL2R(String, _do_something_virtual, String, int);
String test_virtual_implemented_in_script(const String &p_name, int p_value);

String test_use_engine_singleton() const;
};

VARIANT_ENUM_CAST(Example::Constants);
Expand Down
Loading