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

C#: Avoid Dispose until after every notification #84013

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

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Oct 26, 2023

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.

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.

Also documents the flags in CSharpInstance that are somewhat related.
@raulsntos raulsntos added this to the 4.2 milestone Oct 26, 2023
@raulsntos raulsntos requested a review from a team as a code owner October 26, 2023 17:12
@magian1127
Copy link
Contributor

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)

@raulsntos
Copy link
Member Author

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 #78157 would fix it. Can you test both PRs together and see if you can still reproduce the error?

@magian1127
Copy link
Contributor

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

@YuriSizov YuriSizov requested a review from RedworkDE October 30, 2023 14:56
@magian1127
Copy link
Contributor

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.

@akien-mga
Copy link
Member

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

@raulsntos
Copy link
Member Author

Let's go with #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.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 9, 2023
@paulloz
Copy link
Member

paulloz commented May 1, 2024

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

@KoBeWi KoBeWi modified the milestones: 4.3, 4.4 Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C#: Script methods bound to tree_exited / _ExitTree() can't be found.
5 participants