Hey, thanks for the d4f41cc fix on issue #305, that one is confirmed in place on master. I was looking through the reload path again because a user of my extension is still seeing /duels reload leave the server in a broken state on 7.2. I have not had time to verify these on a live server yet, so please treat the following as suspicions and not confirmed bugs. Would appreciate a sanity check from you.
1. unload() ClassLoader filter may be backwards
File: duels-plugin/src/main/java/com/meteordevelopments/duels/DuelsPlugin.java line 178
listener.getClass().getClassLoader().getClass().isAssignableFrom(ExtensionClassLoader.class)
This reads as "is the listener's loader class a supertype of ExtensionClassLoader". For a real extension listener the loader's class is ExtensionClassLoader (or a subclass), so this expression is only true when it is the exact class and false for subclasses. I think the intent is the opposite direction, something like:
ExtensionClassLoader.class.isAssignableFrom(listener.getClass().getClassLoader().getClass())
or equivalently listener.getClass().getClassLoader() instanceof ExtensionClassLoader.
If I am reading this right, extension listeners are never unregistered on unload(), they keep firing against the fresh plugin state after load() runs again. That would explain ghost listeners, NPEs, and double-handled events after a reload.
Could you check whether this branch is actually entered for typical extension listeners on your end? It is possible I am misreading the chain.
2. reload() does not re-run pre-listener and command registration
onEnable() calls initLoadables(), registerAllCommands(), loadExtensions(), and loadPreListeners(). reload() only calls unload() followed by load(). load() iterates loadables and calls handleLoad() on each, but the pre-listeners and the command registrations are not Loadables, so they never come back after a reload.
Specifically:
loadPreListeners() registers KitEditListener and ten other listeners. After unload() calls HandlerList.unregisterAll(this) those are gone and never re-added.
unregisterPluginCommands() strips /duel, /party, /queue, /spectate, /duels, /kit from the CommandMap, but reload() does not call registerAllCommands() again.
If this is what is happening, then after the first /duels reload the user loses the Duels commands and a bunch of listener-driven behavior, even if everything else loaded cleanly. That matches what my user is describing.
Easiest fix would be to also call loadPreListeners() and registerAllCommands() from inside reload() after load() succeeds. Or move them into the Loadable chain so the existing flow handles them.
3. Minor: loadExtensions() failure path can re-introduce #305
loadExtensions() catches its own exception and continues. The d4f41cc line that sets lastLoad = loadables.indexOf(extensionManager) runs after handleLoad(). If handleLoad() throws, lastLoad is never advanced and the next reload skips ExtensionManager.handleUnload() again, which is the original #305 symptom in a failure scenario.
Probably worth setting lastLoad immediately after adding extensionManager to loadables, before calling handleLoad().
Versions tested against: master at HEAD (and 7.2 on Modrinth, same code as far as I can tell). All three findings are based on static reading only, not a reproduction. If you want, I can try to repro 1. and 2. on a test server next week and report back with logs.
Hey, thanks for the d4f41cc fix on issue #305, that one is confirmed in place on master. I was looking through the reload path again because a user of my extension is still seeing
/duels reloadleave the server in a broken state on 7.2. I have not had time to verify these on a live server yet, so please treat the following as suspicions and not confirmed bugs. Would appreciate a sanity check from you.1.
unload()ClassLoader filter may be backwardsFile:
duels-plugin/src/main/java/com/meteordevelopments/duels/DuelsPlugin.javaline 178This reads as "is the listener's loader class a supertype of ExtensionClassLoader". For a real extension listener the loader's class is
ExtensionClassLoader(or a subclass), so this expression is only true when it is the exact class and false for subclasses. I think the intent is the opposite direction, something like:or equivalently
listener.getClass().getClassLoader() instanceof ExtensionClassLoader.If I am reading this right, extension listeners are never unregistered on
unload(), they keep firing against the fresh plugin state afterload()runs again. That would explain ghost listeners, NPEs, and double-handled events after a reload.Could you check whether this branch is actually entered for typical extension listeners on your end? It is possible I am misreading the chain.
2.
reload()does not re-run pre-listener and command registrationonEnable()callsinitLoadables(),registerAllCommands(),loadExtensions(), andloadPreListeners().reload()only callsunload()followed byload().load()iteratesloadablesand callshandleLoad()on each, but the pre-listeners and the command registrations are not Loadables, so they never come back after a reload.Specifically:
loadPreListeners()registers KitEditListener and ten other listeners. Afterunload()callsHandlerList.unregisterAll(this)those are gone and never re-added.unregisterPluginCommands()strips/duel,/party,/queue,/spectate,/duels,/kitfrom the CommandMap, butreload()does not callregisterAllCommands()again.If this is what is happening, then after the first
/duels reloadthe user loses the Duels commands and a bunch of listener-driven behavior, even if everything else loaded cleanly. That matches what my user is describing.Easiest fix would be to also call
loadPreListeners()andregisterAllCommands()from insidereload()afterload()succeeds. Or move them into the Loadable chain so the existing flow handles them.3. Minor:
loadExtensions()failure path can re-introduce #305loadExtensions()catches its own exception and continues. The d4f41cc line that setslastLoad = loadables.indexOf(extensionManager)runs afterhandleLoad(). IfhandleLoad()throws,lastLoadis never advanced and the next reload skipsExtensionManager.handleUnload()again, which is the original #305 symptom in a failure scenario.Probably worth setting
lastLoadimmediately after addingextensionManagertoloadables, before callinghandleLoad().Versions tested against: master at HEAD (and 7.2 on Modrinth, same code as far as I can tell). All three findings are based on static reading only, not a reproduction. If you want, I can try to repro 1. and 2. on a test server next week and report back with logs.