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

[#693] Fix resource leak warnings for org.eclipse.cdt.core.utils #694

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

ruspl-afed
Copy link
Member

@ruspl-afed ruspl-afed commented Feb 6, 2024

  • use try-with-resources for AutoCloseable

Fixes #693

….utils

* use `try-with-resources` for `AutoCloseable`
@ruspl-afed ruspl-afed requested a review from jld01 February 6, 2024 06:13
@ruspl-afed
Copy link
Member Author

ruspl-afed commented Feb 6, 2024

@jld01 we have more hidden resource leak issues near Dwarf and related types when exposing them via ISymbolReader adapter: it is really not clear when to close them properly. Idea to override finalize is not good for Java, since the usage of this method was deprecated long ago. But these seem to be out of scope here, since most probably the proper fix will cause breaking API changes.

Copy link
Contributor

@jld01 jld01 left a comment

Choose a reason for hiding this comment

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

These changes look positive, @ruspl-afed, but I'm struggling to understand why the dispose() methods are necessary in the first place. For example, the Dwarf section maps should fall out of scope with each Dwarf instance. Do you have any insight into what is preventing the garbage collector from doing its job here?

@ruspl-afed
Copy link
Member Author

Do you have any insight into what is preventing the garbage collector from doing its job here?

TBH I have no idea why people were struggling with essential things here. Now I even not sure why we need to have it AutoClosable. Perhaps, they had some streams inside to be closed.

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

The history of why it needs to be autocloseable and not wait until gc runs is in #132

But the short answer is that all these classes hold onto real files so waiting until gc is too late.

@jld01
Copy link
Contributor

jld01 commented Feb 7, 2024

Thanks for the explanation, @jonahgraham. 👍

@ruspl-afed ruspl-afed merged commit 7f69191 into eclipse-cdt:main Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix resource leak warnings for org.eclipse.cdt.core.utils
3 participants