godot icon indicating copy to clipboard operation
godot copied to clipboard

C#: Avoid `Dispose` until after every notification

Open raulsntos opened this issue 1 year ago • 7 comments

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.

raulsntos avatar Oct 26 '23 17:10 raulsntos

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)

magian1127 avatar Oct 28 '23 05:10 magian1127

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?

raulsntos avatar Oct 29 '23 12:10 raulsntos

Okay, I'll test it in the next few days.

magian1127 avatar Oct 30 '23 11:10 magian1127

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.

magian1127 avatar Oct 31 '23 13:10 magian1127

#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.

akien-mga avatar Nov 08 '23 17:11 akien-mga

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.

raulsntos avatar Nov 08 '23 20:11 raulsntos

Do we want to push this to 4.4 then, and find a good way to test reliably for potential memory issues?

paulloz avatar May 01 '24 17:05 paulloz