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 plugin inter-dependencies by using one class loader for all plugins #2280

Merged
merged 2 commits into from May 25, 2016

Conversation

Projects
None yet
4 participants
@bernd
Member

bernd commented May 24, 2016

We used to create on URL class loader for each plugin. This broke
plugins which depend on classes from other plugins.

Now we are using one class loader for all plugins so they can see each
other. Plugins which need classes from other plugins are working now.

Fix plugin inter-dependencies by using one class loader for all plugins
We used to create on URL class loader for each plugin. This broke
plugins which depend on classes from other plugins.

Now we are using one class loader for all plugins so they can see each
other. Plugins which need classes from other plugins are working now.

@bernd bernd added this to the 2.0.2 milestone May 24, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented May 24, 2016

@bernd Wouldn't this also make ChainingClassLoader obsolete if all plugins (and Graylog itself) were using the same class loader?

}
return plugins.build();
final List<URL> urls = Lists.newArrayList(files).stream()

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

This could simply use Arrays.stream(T[] array) instead of instantiating a new array list and then calling stream() on it.

return plugins.build();
final List<URL> urls = Lists.newArrayList(files).stream()
.filter(File::isFile)

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

We'll lose the debug logging that a file was being skipped this way, but I'm fine with it.

final ServiceLoader<Plugin> pluginServiceLoader = ServiceLoader.load(Plugin.class, classLoader);
return ImmutableSet.<Plugin>builder().addAll(pluginServiceLoader).build();

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

This could simply be written as return ImmutableSet.copyOf(pluginServiceLoader);.

@joschi joschi self-assigned this May 24, 2016

@bernd

This comment has been minimized.

Member

bernd commented May 24, 2016

@bernd Wouldn't this also make ChainingClassLoader obsolete if all plugins (and Graylog itself) were using the same class loader?

Right now there are still two class loaders. Not sure if there are any other implications if we use only one class loader for server and plugins. /cc @kroepke

@kroepke

This comment has been minimized.

Member

kroepke commented May 25, 2016

I guess technically we don't need the second classloader and could instead set the parent (i.e. Graylog's) in the URLClassloader as the chaining classloader really delegates to the parent first anyway.

However, if we don't need to touch all of that now I'd prefer not to. But I guess it's simple to test whether it makes a different. I'm unsure what the consequences for any plugin resources are. So far I haven't seen any conflicts, but that doesn't mean there aren't any somewhere.

@joschi

This comment has been minimized.

Contributor

joschi commented May 25, 2016

Works for me™.

@joschi joschi merged commit de3d322 into 2.0 May 25, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 915 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 403 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@joschi joschi deleted the fix-plugin-class-loader branch May 25, 2016

joschi added a commit that referenced this pull request May 25, 2016

Fix plugin inter-dependencies by using one class loader for all plugi…
…ns (#2280)

We used to create on URL class loader for each plugin. This broke
plugins which depend on classes from other plugins.

Now we are using one class loader for all plugins so they can see each
other. Plugins which need classes from other plugins are working now.
(cherry picked from commit de3d322)
@magicdude4eva

This comment has been minimized.

magicdude4eva commented Jun 29, 2016

This is problematic. The graylog-plugin-map-widget-1.0.3.jar ships with a version of httpclient which is missing for example URIBuilder:

[root@gls1 plugin]$ jar -tvf graylog-plugin-map-widget-1.0.3.jar | grep -i "http/client/utils"
     0 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/
  1957 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/JdkIdn.class
   938 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/Punycode.class
   177 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/Idn.class
  1913 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/CloneUtils.class
  3017 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/Rfc3492Idn.class
  4526 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/URIUtils.class
  5214 Mon Jun 20 15:27:24 SAST 2016 org/apache/http/client/utils/URLEncodedUtils.class

My plugin can now not ship with httpclient as it conflicts with some classes in the map-plugin, but can also not ship without httpclient as org/apache/http/client/utils/URIBuilder is missing from the map plugin.

I can now try to "hack" together my plugin to include classes which are missing from the map-plugin as I can not include httpclient or I can ask users to completely remove the map-plugin from the plugins folder and then ship my plugin as before with httpclient and httpmime.

In my case I am using jira-client which uses URIBuilder which is pulled from my bundled version of httpclient, but then references

java.lang.NoSuchMethodError: org.apache.http.client.utils.URLEncodedUtils.encPath(Ljava/lang/String;Ljava/nio/charset/Charset;)Ljava/lang/String;
    at org.apache.http.client.utils.URIBuilder.encodePath(URIBuilder.java:191) ~[graylog-plugin-jira-1.0.7.jar:?]
    at org.apache.http.client.utils.URIBuilder.buildString(URIBuilder.java:152) ~[graylog-plugin-jira-1.0.7.jar:?]
    at org.apache.http.client.utils.URIBuilder.build(URIBuilder.java:120) ~[graylog-plugin-jira-1.0.7.jar:?]
    at net.rcarz.jiraclient.RestClient.buildURI(RestClient.java:117) ~[graylog-plugin-jira-1.0.7.jar:?]

Alternatively, could the map-widget at least include versions of httpclient which are more up-to-date than 4.0.1?

@kroepke

This comment has been minimized.

Member

kroepke commented Jun 29, 2016

@magicdude4eva We can update it, but for various reasons we need the plugins to share a classloader.
Unfortunately, without going an OSGi route (shudder), there's not going to be an easy solution.

Can you shade/relocate that specific dependency in your plugin? That should provide a workaround.

@magicdude4eva

This comment has been minimized.

magicdude4eva commented Jun 29, 2016

I can try shade/relocate and hope that with the various dependencies https://github.com/rcarz/jira-client will still work (which I need for my plugin https://github.com/magicdude4eva/graylog-jira-alarmcallback).

Just to confirm: Was the classloader change for plugins introduced with 2.0.3 (I am currently running 2.0.3 and can not recall to having the issue before).

I think the big problem with one classloader is that any plugin could break anyone else and then it becomes plugin authors responsibility to try and fix a workaround.

If anyone can find a clever workaround for my problem, feel free to assist with the pom (I am honestly struggling with this as I am not very familiar with maven builds - https://github.com/magicdude4eva/graylog-jira-alarmcallback).

For now I will just ask users to delete the map-plugin as I have no time to try and find a work-around for this in the next week or two.

@kroepke

This comment has been minimized.

Member

kroepke commented Jun 29, 2016

This was changed in 2.0.2 to fix an issue with our commercial plugins.

I realize that a common classloader can pose challenges but the alternatives require immense complexity as well.
We can potentially change the way this works by allowing plugins to specify which classloader isolation they want.

Regarding the jira library, the only thing I see that could be problematic is that it uses a really old Jodatime version. It's the same issue with the httpclient being pulled in by the map widget: the old dependency actually comes in transitively which makes it difficult to update it, because upstream, third parties need to update, in this case MaxMind.

I'll start a new issue to track and discuss this.
Thanks for letting us know!

@magicdude4eva

This comment has been minimized.

magicdude4eva commented Jun 29, 2016

Thank you @kroepke. One more question before I try another work-around:

When looking at the server log it shows that my "JIRA" plugin is loaded first:

2016-06-29T15:19:14.530+02:00 INFO  [CmdLineTool] Loaded plugin: JIRA integration plugin 1.0.7 [com.bidorbuy.graylog.alarmcallbacks.jira.JiraAlarmCallback]
2016-06-29T15:19:14.532+02:00 INFO  [CmdLineTool] Loaded plugin: Collector 1.0.3 [org.graylog.plugins.collector.CollectorPlugin]
2016-06-29T15:19:14.532+02:00 INFO  [CmdLineTool] Loaded plugin: Enterprise Integration Plugin 1.0.3 [org.graylog.plugins.enterprise_integration.EnterpriseIntegrationPlugin]
2016-06-29T15:19:14.532+02:00 INFO  [CmdLineTool] Loaded plugin: Pipeline Processor Plugin 1.0.0-beta.5 [org.graylog.plugins.pipelineprocessor.ProcessorPlugin]
2016-06-29T15:19:14.533+02:00 INFO  [CmdLineTool] Loaded plugin: Anonymous Usage Statistics 2.0.3 [org.graylog.plugins.usagestatistics.UsageStatsPlugin]

Would my assumption be correct, that since I bundle a newer version of httpclient, that if I just do a "dummy" instantiation of HTTPClient classes in a static initialiser block of my AlarmCallBack-class that this would then trigger the loading of my httpclient classes and then breaking the map-plugin?

@kroepke

This comment has been minimized.

Member

kroepke commented Jun 29, 2016

The load order is pretty much undefined, so I wouldn't rely on it.

Whether or not the map plugin breaks depends on the binary compatibility of the different versions, that's tricky to say.
What I've done in the past to work around issues like these was to include the library's code as a git submodule (from a fork) and then update the library to use a specific version.
That's messy and requires tracking changes, but is often the only sane way to deal with jar hell.

@magicdude4eva

This comment has been minimized.

magicdude4eva commented Jun 29, 2016

I tried for the last 4 hours to work around this issue, but there is no good way solving it without hacking the build and it will be just a matter of time that another user has similar issues.

For me personally it is okay to delete the map-plugin as I don't use it, but I think other users might have similar issues when moving to Graylog 2.0.2/2.0.3.

@kroepke

This comment has been minimized.

Member

kroepke commented Jun 29, 2016

That is true, if we can find a better way that's not too intrusive (and find the time to make it work) my hope is to have a solution in time for 2.1.

bernd added a commit that referenced this pull request Jul 20, 2016

Make it possible for plugins to request a shared class loader
In #2280 we changed the plugin loader so that all plugins share one
class loader and they can see each other. (needed for plugin
inder-dependencies)

This is problematic when different plugins have conflicting
dependencies. (see #2436)

With this change, every plugin gets its own class loader by default so
we can avoid dependency clashes. Plugins which depend on other plugins
can request a shared class loader via a graylog-plugin.properties file.

Fixes #2436

bernd added a commit that referenced this pull request Jul 20, 2016

Make it possible for plugins to request a shared class loader
In #2280 we changed the plugin loader so that all plugins share one
class loader and they can see each other. (needed for plugin
inter-dependencies)

This is problematic when different plugins have conflicting
dependencies. (see #2436)

With this change, every plugin gets its own class loader by default so
we can avoid dependency clashes. Plugins which depend on other plugins
can request a shared class loader via a graylog-plugin.properties file.

Fixes #2436

bernd added a commit that referenced this pull request Jul 25, 2016

Make it possible for plugins to request a shared class loader
In #2280 we changed the plugin loader so that all plugins share one
class loader and they can see each other. (needed for plugin
inter-dependencies)

This is problematic when different plugins have conflicting
dependencies. (see #2436)

With this change, every plugin gets its own class loader by default so
we can avoid dependency clashes. Plugins which depend on other plugins
can request a shared class loader via a graylog-plugin.properties file.

Fixes #2436

kroepke added a commit that referenced this pull request Jul 28, 2016

Make it possible for plugins to request a shared class loader (#2508)
* Make it possible for plugins to request a shared class loader

In #2280 we changed the plugin loader so that all plugins share one
class loader and they can see each other. (needed for plugin
inter-dependencies)

This is problematic when different plugins have conflicting
dependencies. (see #2436)

With this change, every plugin gets its own class loader by default so
we can avoid dependency clashes. Plugins which depend on other plugins
can request a shared class loader via a graylog-plugin.properties file.

Fixes #2436

* Adjust log messages in PluginLoader

* Move properties file into a subdirectory to avoid clashes

The path to the properties file is stored in the
"Graylog-Plugin-Properties-Path" attribute of the JAR manifest.

* Only create shared class loader if any plugin requests to be shared
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment