I don’t see any point in the current PluginClassLoader usage - in fact it’s a really bad thing to lump all the plugin lib directories together and can lead to real support issues
Well, I am not sure if this is bad or not, but IMHO there should be a way to make Spark container aware of what plugins are installed. Loading all plugins in the same class loader proves to be a solution… What this doesn’t seem to resolve are plugin dependencies (other plugin jars), as you noticed.
I think that there are three major aspects here:
- There is Spark - the plugin container
- There are plugins
- There are plugin dependencies.
What we have a this point:
For 1 - there is the the Spark class loader responsible for spark libraries loading
For 2 - there is the Plugin class loader that loads all plugin jars (lib directories)
In addition for point 3 What you need is to load plugin dependencies in dedicated class loaders for each plugin. All those class loaders should inherit the generic plugin class loader
IMO, there is no need to beak the current functionality, removing the clas loader from point 2 I think will break the plugin dependency concept (in plugin.xml you can put a dependency tag and make one plugin to be loaded after a parent plugin)
I am not sure I understand the Container’s purpose of being ‘aware’ of what plugins are installed and what problem is solved by adding the lib directories to the classpath. By doing this, it effectively makes every plugin a runtime dependency of every other, but without the benefit of being able to define the dependecy order as can currently be done. Sure, the container can know what plugins are installed by parsing the plugins directory, but if this is to solve the dependency issue, it is too crude.
IMHO, it seems like the container’s runtime is being polluted with too much knowledge of the Plugins. The container needs to be simply able to parse and load the plugins runtime descriptors and then launch them when appropriate. It should not have a runtime i.e. classpath, awareness. Tomcat container does not have all the webapp classes/libs on its own classpath.
When debugging yesterday, I found the plugin dependency support, which I couldn’t find documented anywhere in the SparkPlug dev guide, but there is a simple solution to that dependency issue rather than having a common class loader that is aware of all libs.
Yesterday, during initializePlugins, after it does the dependency checking, I just then ran through each Plugin § again and added all the jar files from the plugin dependency (D) to the PluginClassLoader instance for the Plugin §, so I have a working solution now that gives the following
PluginManager creates a new PluginClassLoader before creating the Plugin class instance. This classloader is used to load and create the Plugin instance, so all classes then created by the Plugin will be automatically loaded by this loader.
PublicPlugin is given the class loader instance (if it wants it)
PluginClassLoader is populated with URLs in this order
a) plugins/pluginName/classes - directory
So, this would seem to support the class segregation as well as the dependency and should not break anything. One possible improvement is that the PluginManager could have the plugins ‘shared’ directory as a classpath, where at the moment I have added them © & (d) to each plugin specific loader.
BTW, I noticed also that the addPlugin() method used when downloading a plugin does not handle any dependencies. I am not sure if dependecies are possible from downloaded plugins, but given that the addPlugin() is public, it should take care of dependencies if there are any.
I was looking on how Tomcat class loading takes place: The Overview Diagram:
The plugin class loader created by spark (point 2) is somehow similar with “Shared” class loader from above diagram. What is missing right now are class loaders below Shared
That link is for Tomcat 3.2.4/4, which is 10 years old - I was working with Tomcat classloader issues in 2001 where they had similar problems - I even found one of my old messages about log4j problems . Current Tomcat dev is 7 (also 6), but the classloaders hierarchy is similar - http://tomcat.apache.org/tomcat-7.0-doc/class-loader-howto.html but has developed since then,
If you like, I can upload a patch so you can see where I am going with this.