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

Prevent duplicate loading of plugins in PluginLoader #948

Merged
merged 2 commits into from Feb 10, 2015

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Feb 5, 2015

When plugins were available multiple times on the class path or in the plugin directory of Graylog, the PluginLoader used to load identical plugins multiple times. By using a sorted set and a specialized Comparator in PluginLoader we prevent that behaviour.

@joschi joschi force-pushed the plugin-loader branch from c75d3b2 to 2cd8e70 Feb 5, 2015

final Set<Plugin> sorted = ImmutableSortedSet.orderedBy(new PluginComparator())
.addAll(loadClassPathPlugins())
.addAll(loadJarPlugins())
.build();

This comment has been minimized.

@kroepke

kroepke Feb 10, 2015

Member

Will this always add the classpath plugins before the jar plugins and always load the oldest plugin version?

I think it should be the other way around, allowing jar plugins to overlay classpath one's and later plugin versions to be preferred.

This comment has been minimized.

@joschi

joschi Feb 10, 2015

Contributor

The Plugin interface doesn't require equals and hashcode and if plugin authors don't override those methods, the set could still contain duplicates (i. e. same plugin in different versions) as the default method implementations in Object will be used.

This comment has been minimized.

@joschi

joschi Feb 10, 2015

Contributor

As a matter of fact, the plugins in the classpath will also be contained in the collection returned by loadJarPlugins() because the same classloader is used as parent classloader for the URLClassLoader instances created in that method.

This comment has been minimized.

@kroepke

kroepke Feb 10, 2015

Member

fair enough, let's address this in a later iteration, then.

plugins.addAll(loadJarPlugins());
final ImmutableSet.Builder<Plugin> unique = ImmutableSet.builder();
for (Plugin plugin : sorted) {
unique.add(new PluginAdapter(plugin));

This comment has been minimized.

@kroepke

kroepke Feb 10, 2015

Member

same as above?
why do we need a second set here?

This comment has been minimized.

@joschi

joschi Feb 10, 2015

Contributor

The PluginAdapter class takes care of eliminating duplicate plugins (in different versions) by only taking the unique ID and the name of the plugins into account for equals() and hashcode(). The sorted set can still contain duplicates (as described in #948 (comment)).

This comment has been minimized.

@kroepke

kroepke Feb 10, 2015

Member

ah, missed that comment.

@kroepke

This comment has been minimized.

Member

kroepke commented Feb 10, 2015

I think that a plugin's name is only used for display purposes, right?

If that's true, then I think the name itself should not be used for comparisons, instead we should define what the unique id should look like.
I would say we should advise people to use a package or class name, like the Riemann output plugin does: https://github.com/Graylog2/graylog2-plugin-output-riemann/blob/master/src/main/java/org/graylog2/outputs/riemann/RiemannOutputMetadata.java#L14

@kroepke kroepke self-assigned this Feb 10, 2015

@kroepke

This comment has been minimized.

Member

kroepke commented Feb 10, 2015

otherwise lgtm 👍

@joschi joschi force-pushed the plugin-loader branch from 27ea0b6 to f308735 Feb 10, 2015

@joschi joschi added this to the 1.0.0 milestone Feb 10, 2015

Prevent duplicate loading of plugins
When plugins were available multiple times on the classpath or in the plugin directory
of Graylog, the PluginLoader used to load identical plugins multiple times. By using a
sorted set and a specialized Comparator<Plugin> in PluginLoader we prevent that behaviour.

Fixes #946

@joschi joschi force-pushed the plugin-loader branch from f308735 to 15dda63 Feb 10, 2015

joschi added a commit that referenced this pull request Feb 10, 2015

Merge pull request #948 from Graylog2/plugin-loader
Prevent duplicate loading of plugins in PluginLoader

Fixes #946

@joschi joschi merged commit 23a2eff into 1.0 Feb 10, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@joschi joschi deleted the plugin-loader branch Feb 10, 2015

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