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.

Sign up for GitHub

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

Jump to bottom

close JarFile stream after getManifest #197

Merged
merged 4 commits into from
Jan 23, 2018
Merged

close JarFile stream after getManifest #197

merged 4 commits into from
Jan 23, 2018

Conversation

hxzhao527
Copy link
Contributor

According to the Plugin lifecycle, a plugin stopped should be able to be removed. But it couldn't. Threre still is some stream opened.

One is in the PluginDescriptor, the other is the URLClassLoader.

@coveralls
Copy link

coveralls commented Jan 22, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.03%) to 57.356% when pulling 86d1d75 on HeartUnchange:master into a78fe6d on decebals:master.

decebals
decebals reviewed Jan 22, 2018
View reviewed changes
pf4j/src/main/java/org/pf4j/ManifestPluginDescriptorFinder.java
@@ -51,8 +51,8 @@ public PluginDescriptor find(Path pluginPath) throws PluginException {

protected Manifest readManifest(Path pluginPath) throws PluginException {
if (FileUtils.isJarFile(pluginPath)) {
try {
Manifest manifest = new JarFile(pluginPath.toFile()).getManifest();
try(JarFile jar = new JarFile(pluginPath.toFile())) {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@decebals
Copy link
Member

@HeartUnchange
The fix for ManifestPluginDescriptorFinder is good.
The second fix, for AbstractPluginManager.stopPlugins() I think that it's not good. You can delete a plugin only after you unload the plugin (https://github.com/decebals/pf4j/blob/master/pf4j/src/main/java/org/pf4j/PluginManager.java#L106).
In unloadPlugin method (https://github.com/decebals/pf4j/blob/master/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java#L281) we close the class loader.
The idea is that after a stop plugin you can call start plugin and the same class loader is used.
Actually, it's a deletePlugin method (https://github.com/decebals/pf4j/blob/master/pf4j/src/main/java/org/pf4j/PluginManager.java#L130) that delete a plugin from system (inclusive the jar file).

I think that it's a good idea to submit the fix for ManifestPluginDescriptorFinder from a separate PR (pull request) and I will merge it.
After this we can comment here, on the second submit, if you believe that your contribution for stopPlugins is OK.

@hxzhao527
Copy link
Contributor Author

I am sorry. I didn't notice the unloadPlugin method. Just change ManifestPluginDescriptorFinder now. About the stopPlugins , I will create a new issue.

@hxzhao527 hxzhao527 changed the title delete plugin jar after method stopPlugins called close JarFile stream after getManifest Jan 23, 2018
@decebals decebals merged commit e80aae6 into pf4j:master Jan 23, 2018
@decebals
Copy link
Member

Thanks!

@jgomer2001 jgomer2001 mentioned this pull request May 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@decebals decebals decebals left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hxzhao527 @coveralls @decebals