godot
godot copied to clipboard
C#: Avoid `Dispose` until after every notification
Instead of disposing the C# instance when receiving NOTIFICATION_PREDELETE
which may not be the last notification sent, we now assume the CSharpInstance destructor will always be called right before destroying the Object with the script and after every notification has already been sent.
- Fixes https://github.com/godotengine/godot/issues/82279.
- This PR is an alternative of https://github.com/godotengine/godot/pull/83670 that doesn't require adding a notification, but I feel like it's more risky :warning:. It may create a memory leak, and needs to be more exhaustively tested.
When exiting the game, this error is occasionally encountered. No other problems have been found.
ERROR: FATAL: Condition "csharp_lang && !csharp_lang->script_bindings.is_empty()" is true.
ERROR: 3 RID allocations of type 'class GodotShape2D * __ptr64' were leaked at exit.
at: CSharpLanguage::_instance_binding_free_callback (modules\mono\csharp_script.cpp:1337)
Thanks for testing, I haven't been able to reproduce the error you mention but it doesn't sound surprising. It seems, since we are no longer disposing the scripts, some may remain after GDMono is freed. If that's the case, maybe https://github.com/godotengine/godot/pull/78157 would fix it. Can you test both PRs together and see if you can still reproduce the error?
Okay, I'll test it in the next few days.
After merging #78157, the previous error did not occur again. I used the Godot editor for development for a day and did not encounter any errors related to these two PRs.
#83670 is approved, so we could go with that PR instead of this approach. But since this one also got tested successfully, I'll let you assess which one you think is best @raulsntos.
Let's go with https://github.com/godotengine/godot/pull/83670 then, I think it's safer and we're too close to the 4.2 release. This PR may be the better solution long-term, but I think it needs to be tested more widely.
Do we want to push this to 4.4 then, and find a good way to test reliably for potential memory issues?