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

Dispose Glfw/Sdl When Checking if Applicable #2352

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Conversation

Aragas
Copy link
Contributor

@Aragas Aragas commented Nov 11, 2024

Summary of the PR

Calling GLFW.Glfw.GetApi(); when Glfw is applicable without disposing it leaks the native module. So we make sure Dispose() is called, which triggers freeing of the native library and reduce the 'counter'.

Further Comments

This was discovered when I was trying to delete the native library (glfw3.dll) on program exit. The delete failed because the program was still referencing the module.
The codebase uses the lazy GlfwProvider.GLFW where it makes sense. When needed, the lazy implementation will have the Dispose() called, thus freeing glfw3. But because of the applicable check, the module will never truly be freed without calling GlfwProvider.GLFW.Value._ctx.Dispose(); manually at least once after a Window creation

@Aragas Aragas requested a review from a team as a code owner November 11, 2024 11:50
@Aragas Aragas changed the title Dispose Glfw when Checking if Applicable Dispose Glfw When Checking if Applicable Nov 11, 2024
@Perksey
Copy link
Member

Perksey commented Nov 11, 2024

Good spot! Would it be possible to make this change in SdlPlatform as well?

@Aragas Aragas changed the title Dispose Glfw When Checking if Applicable Dispose Glfw/Sdl When Checking if Applicable Nov 11, 2024
@Aragas
Copy link
Contributor Author

Aragas commented Nov 11, 2024

Good spot! Would it be possible to make this change in SdlPlatform as well?

Good point, done

@Aragas
Copy link
Contributor Author

Aragas commented Nov 11, 2024

@dotnet-policy-service agree

Copy link
Member

@Perksey Perksey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Perksey Perksey enabled auto-merge (squash) November 11, 2024 12:16
@Perksey Perksey merged commit 544b647 into dotnet:main Nov 11, 2024
5 checks passed
@Aragas Aragas deleted the patch-1 branch November 11, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants