In the current 1.2.5-R4 Recommended Build, plugins are disabled in reverse order from when they were enabled (builds #2205 onward). So, I'd like some feedback on your experiences with the previous disable order in comparison to the new disable order. The changes were designed to address issues with class loaders (which were made predictable in the latest recommended build, 1.2.5-R3.0) and dependencies not being available during disable routines.
I had the same thoughts, md_5. Seems like a perfectly reasonable and necessary change and will definitely help in some cases in my own plugin dependencies. I don't see why this needs to be discussed?
All plugins I have made for private use (haven't released any with dependencies) listen for the plugins they depend on disabling and disable themselves. Would this break the order as it would hence disable too early? (obviously the other plugins load first because of depend: [Plugin]
This should make the actual order it gets disabled similar to what you are doing already, without the need to manually listen for the disable events. This serves both as a warning and a means for plugin authors to speak up about the new order (especially for authors that make suites).
I'm not sure whether reversing the disabling order solves the problem of missing classes. I'll test it. But I guess if you have two plugins Plugin and Plugin_Addin. Plugin_Addin depends on Plugin. Then the code could contain a list of "Saveable" Objects. Some of them may be added in Plugin_Addin (being a class defined in Plugin_Addin), but are saved in Plugin. This may cause issues. I thought about another solution. (The ticket is mine) You could create a HashMap entry on pluign loading Code: protected static Map<String,String> depencies=new Map<String,String>(); pluginEnable { [...] foreach (depency) depencies.get(depency.getName()).add(this.getName()); depencies.put(this.getName(),new List<String>()); [...] } pluginDisable { [...] foreach (depency) { depencies.get(depency.getName()).remove(this.getName()); if ( depencies.get(depency.getName()).size()==0) { depency.unloadClasses(); depencies.remove(depency.getName()); } } if (depencies.get(this.getName()).size()==0) { unloadClasses(); depencies.remove(this.getName()); } } Shall i create a pullrequest with proper code?
Effectively, that's what's happening now, because dependencies are causing a plugin to load with dependent after depended, it will unload dependent before depended. The issue with depended saving classes from something getting disabled is not an issue. The class loader design will always do a self-reference to finally find a class (in this case, the calling classloader is really the one of the orphaned plugin). TL;DR, there should be no further issues unless there is an incorrect dependency hierarchy.
Maybe I got you wrong, but this example should fail. Cannot test it properly because class unloading happens in another thread I guess. Plugin1: P1Main+Saveable Plugin2 depends on Plugin1: P2Main+Storage implements Saveable+SaveHelper P1Main: static getP1Main() addSaveable(saveable); onDisable() { for (Saveable save:savelist) save.save(config); } P2Main: onEnable() { getP1Main().addSaveable(new Storage()); } Storage: save(config) { help=new SaveHelper(); help.foo(); config.set("help",help.toString()); config.set(this.getName(),this.toString()); } Old disable order: Should cause no errors New disable order: Storage + SaveHelper already disabled (ERROR) My version (with old order) no errors Or does this plugin something wrong?
With the new version, Plugin2.Storage is referenced from Plugin1. Plugin1 executes method on Plugin2, but Plugin2 is disabled. Anything referenced from Plugin2.Storage will still have a fallback to the Plugin2.class.getClassLoader(). Plugin1 cannot 'directly' access anything in Plugin2 at this state, but Plugin2 objects have their own 'scope' as defined by the class loader, and the scope will still fall back to its own classloader (now dereferenced from JavaPluginLoader). If you cannot 'save' after you have been disabled, as far as your implementation, then that should be corrected with the new disable order in mind.
