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

Fix the use of Object::_instance_bindings when GDExtensions are uninitialized #101466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adamscott
Copy link
Member

Fixes #101465

@adamscott adamscott added this to the 4.4 milestone Jan 12, 2025
@adamscott adamscott requested a review from a team as a code owner January 12, 2025 17:11
@adamscott adamscott changed the title Fix the use of Object::_instance_bindings when GDExtensions uninit Fix the use of Object::_instance_bindings when GDExtensions are uninitialized Jan 12, 2025
@akien-mga akien-mga requested a review from a team January 12, 2025 17:13
@dsnopek
Copy link
Contributor

dsnopek commented Jan 13, 2025

Thanks!

As I mentioned over on #101465, this is a bug that should have already been fixed in godot-cpp.

However, if that bug has come back, I don't think this is the right solution.

Either (1) bindings need to make sure to clean up their singleton bindings when unloading (which is what godotengine/godot-cpp#1458 aimed to do in godot-cpp) or (2) we should check the GDExtensionManager to see if the extension is still loaded before trying to run the callbacks.

@adamscott
Copy link
Member Author

However, if that bug has come back, I don't think this is the right solution.

Either (1) bindings need to make sure to clean up their singleton bindings when unloading (which is what godotengine/godot-cpp#1458 aimed to do in godot-cpp) or (2) we should check the GDExtensionManager to see if the extension is still loaded before trying to run the callbacks.

I don't understand.

  1. When the original developer doesn't does it's job correctly, it shouldn't necessarily make Godot crash on exit.
  2. How can we check GDExtensionManager when it's already uninitialized? My PR is all about that situation, when GDExtensionManager is already unloaded.

@dsnopek
Copy link
Contributor

dsnopek commented Jan 15, 2025

When the original developer doesn't does it's job correctly, it shouldn't necessarily make Godot crash on exit.

It isn't the job of the developer, but the language bindings, so godot-cpp in this case. There's lots of cases where if the language bindings don't do what they are supposed to, it will lead to crashes, and I don't think there is much we can do about that.

How can we check GDExtensionManager when it's already uninitialized? My PR is all about that situation, when GDExtensionManager is already unloaded.

It's a little tricky. I wrote a bit about it here:

2. We already associate the callbacks with a void *p_token which is the GDExtension * in the case of GDExtensions, which we could check for in GDExtensionManager. The only problem is that C# also uses these callbacks, and, in that case, the void *p_token is something else entirely. This won't be a problem once C# uses GDExtension, but for now, we'd probably need to track some new piece of data with the binding callbacks? Or, maybe, we can maintain a list of void *p_tokens that are no longer valid that we update when a GDExtension is unloaded, and then we don't run the free_callback if it's on the list?

@adamscott adamscott modified the milestones: 4.4, 4.x Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants