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

fix: let Plugin implement AutoCloseable #2025

Merged
merged 1 commit into from May 4, 2022

Conversation

gtoison
Copy link
Contributor

@gtoison gtoison commented Apr 21, 2022

Proposed fix for #2024

When a custom plugin is loaded from a .jar file PluginLoader creates URLClassLoader and never closes it. This leaves a lock (a RandomAccessFile) on the .jar file and it cannot be deleted or renamed until the JVM process stops.

Keep track of the URLClassLoader created by the PluginLoader so we can close them when we are done with the Plugin

It is up to the code using these custom plugins to call .close() once the analysis is completed. The current code uses static initialization for the core plugin so I couldn't find a clean way (for instance using try-with-resources blocks) to close the plugins.


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

When a custom plugin is loaded from a .jar file PluginLoader creates
URLClassLoader and never closes it. This leaves a lock (a
RandomAccessFile) on the .jar file and it cannot be deleted or renamed
until the JVM process stops.
Keep track of the URLClassLoader created by the PluginLoader so we can
close them when we are done with the Plugin
@KengoTODA
Copy link
Member

Confirmed that Plugin : PluginLoader is 1 : 1 so closing parent (PluginLoader) from child (Plugin) would have less problem.

I guess we have several places to call Plugin.close() in the current codebase, like this and FindBugs2.dispose(), I will recheck again later.

@gtoison
Copy link
Contributor Author

gtoison commented Apr 22, 2022

It might be better to "close" the plugin inside the static method Plugin.removeCustomPlugin() since all the clean-up code is already there.

It seems to me that the lifecycle of the plugin is not the same as the lifecycle of FindBugs2 class: it seems reasonable to load some plugins, run a several analysis, then unload the plugins.
It would be great to close the Plugin loader as soon as the plugin is loaded but there's for instance a call to Plugin.getClassLoader in SAXBugCollectionHandler so I think that the PluginLoader needs to remain alive during the analysis.

In any case you guys know the codebase better than I do, so let me know if this PR needs to be amended

@KengoTODA KengoTODA merged commit 4e61eac into spotbugs:master May 4, 2022
@gtoison gtoison deleted the make-plugin-autocloseable branch May 6, 2022 11:51
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.

None yet

3 participants