Skip to content
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

SystemProperty: Remember the classloader which loaded a class #2000

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sco0ter
Copy link
Contributor

@sco0ter sco0ter commented Feb 14, 2022

Following happened to me:

  1. Set a custom LockoutProvider class which is located in a plugin
  2. During authentication, the LockOutManager.getInstance() was called for the first time and tried to load the custom LockoutProvider class. It failed, because JiveClassLoader could not find it (it's the classloader used in auth code).

This commit sets the classloader whenever a class is set in a property via the setValue() method. When getValue() is called, the classloader associated with the set class can be used to load the class with the correct classloader.

This does not work during the initial start of Openfire if properties are loaded statically, but if a plugin later sets its own class and Openfire code then tries to load this class (in my example during authentication), it can use the correct classloader.

Current workaround is to call LockOutManager.getInstance() in the plugin code, so that it already initialized with the custom lockout provider and does not try to load the class during authentication.

Following happened to me:
1. Set a custom LockoutProvider class which is located in a plugin
2. During authentication, the LockOutManager.getInstance() was called for the first time and tried to load the custom LockoutProvider class. It failed, because JiveClassLoader could not find it (it's the classloader used in auth code).

This commit sets the classloader whenever a class is set in a property via the setValue() method. When getValue() is called, the classloader associated with the set class can be used to load the class with the correct classloader.

This does not work during the initial start of Openfire if properties are loaded statically, but if a plugin later sets its own class and Openfire code then tries to load this class (in my example during authentication), it can use the correct classloader.

Current workaround is to call LockOutManager.getInstance() in the plugin code, so that it already initialized with the custom lockout provider and does not try to load the class during authentication.
@guusdk
Copy link
Member

guusdk commented Feb 14, 2022

Thanks for the PR @sco0ter. Did you check how this behaves around the various stages of a plugin lifecycle? I'm worried that the reference to a (plugin)classloader causes plugins that provide classes that are refenced by this implempentaiton keep them from being garbage collected when they are unloaded/reloaded/updated. This has proven to have particularly nasty side-effects (effectively having two versions of the same plugin partially loaded).

As an aside: battling classloading-related issues with Openfire plugins is often a signal to me to start checking if it is worthwhile to have a particular piece of functionality being implemented as a plugin in the first place. Plugins provide ease of deployment, but can also be unloaded/replaced at runtime. For certain functionality (especially around things like authentication), this might not be very desirable. In such cases, it's worth considering moving the functionality in a plain old non-plugin library / jar file, that's added in Openfire's lib folder. That immediately solves most classloading issues, as such libraries would no longer be loaded by a PluginClassLoader.

@sco0ter
Copy link
Contributor Author

sco0ter commented Feb 21, 2022

No I did not check that. I am afraid, I have no time to investigate into the lifecycle and classloading and garbarge collection stuff.

Thanks for the hint with the lib folder. We avoid messing around with it, though, and keep custom logic in plugins.

@GregDThomas
Copy link
Contributor

In the (distant) past, when I had to do something similar, I had the plugin check the core Openfire class loader (not the default, plugin class loader) for the existence of a class in the custom .jar file. If it wasn't there, you can either shutdown Openfire with logs reporting why, or on the plugin admin screens highlight that the .jar file needs copying from the plugin/lib to the openfire/lib folder and restart Openfire manually.

@akrherz
Copy link
Member

akrherz commented Sep 4, 2023

👋 @sco0ter, do you still have interest pushing this PR over the finish line?

@sco0ter
Copy link
Contributor Author

sco0ter commented Sep 5, 2023

Sorry, no. I am out of business with Openfire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants