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#: Clear instance bindings callbacks on finalizing the language #93172

Merged

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Jun 14, 2024

When finalizing the C# language every C# instance is disposed and the instance bindings callbacks are no longer valid. Clearing the instance bindings ensures these callbacks are not called, and since we dispose of every C# instance there should be no leaks.

⚠️ The linked issues are hard to reproduce so I would appreciate some testing to confirm that it fixes them.

When finalizing the C# language every C# instance is disposed and the instance bindings callbacks are no longer valid. Clearing the instance bindings ensures these callbacks are not called, and since we dispose of every C# instance there should be no leaks.
@raulsntos raulsntos added this to the 4.4 milestone Jun 14, 2024
@raulsntos raulsntos requested a review from a team as a code owner June 14, 2024 18:46
@hhyyrylainen
Copy link

I just saw this PR linked to an issue I reported. If it is still relevant to test this, I'll probably have time next week (unless there's breaking changes from projects using 4.2.2 I would have trouble quickly fixing meaning I couldn't easily test this).

@raulsntos
Copy link
Member Author

Testing is very much appreciated. It'd be great if you could confirm whether the PR fixes your issue.

Regarding breaking changes from 4.2.2. Since this PR is based on 5833f59 which I believe is closest to 4.3-beta.3, you can use the Upgrading from Godot 4.2 to Godot 4.3 documentation page to see the list of breaking changes that you may have to deal with.

@hhyyrylainen
Copy link

This works! The Godot editor version I compiled from this branch no longer crashes on shutdown (just complains about the usual rendering server singleton null). Just to confirm I also compiled a version from latest master branch commit (3e0c10d) and did the exact same steps as in the other test, and that still crashes. So I can confirm that this PR seems to fix the crash I reported: #89941

I didn't do a really thorough test but nothing major was obviously wrong with my game with this PR so this doesn't break at least anything immediately obvious. I did notice that I'll have the task of fixing some icon sizing again with the Godot 4.3 release, but that's not related to the changes in this PR.

@LeonardoDemartino
Copy link
Contributor

I have tested this PR and fixes #95708, +1 to merge this ASAP please. 😃

@akien-mga akien-mga merged commit 3647bc3 into godotengine:master Aug 19, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/clear-instance-bindings branch August 19, 2024 13:51
@AlexanderLangguth
Copy link

Great that you found and fixed the issue.
Is there any chance this will be included in a patch release? Cause it breaks our CI pipelines.

@raulsntos
Copy link
Member Author

@AlexanderLangguth This is a bit of a risky change so I'd rather wait until it gets more testing on 4.4 first, to ensure it doesn't introduce regressions (e.g.: memory leaks). Can you confirm that your CI issues are fixed with Godot 4.4-dev.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment