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

Dynamically update classloader after adding a library with the add-library command #25153

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

Conversation

OndroMih
Copy link
Contributor

Dynamically updates the common classloader if a common library is added. It means that restart is not needed to add the library to the classpath.

Dynamically updates the common classloader if a common library is added.
* @throws UnsupportedOperationException If adding not supported by the classloader
*/
public void addToClassPath(URL url) {
if (commonClassLoader != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just invert the condition, == null should be first.

@dmatej
Copy link
Contributor

dmatej commented Sep 23, 2024

Is it possible also to remove the library? I see there is the asadmin remove-library command too, however I have no idea how it works if the application is already running and using the library.

@dmatej dmatej added this to the 7.0.18 milestone Sep 23, 2024
@dmatej dmatej added the New feature A major new user functionality was added label Sep 23, 2024
@dmatej dmatej requested a review from a team September 23, 2024 19:44
public String getCommonClassPath() {
return commonClassPath;
}

private static String urlsToClassPath(Stream<URL> urls) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a great candidate to have unit test for!

return urls
.map(URL::getFile)
.filter(Predicate.not(String::isBlank))
.map(file -> new File(file).getAbsolutePath())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the lambda be replaced with additional .map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean have 2 map calls instead of just one? It could but I thought it’s an unnecessary overhead to call map twice.

@avpinchuk
Copy link
Contributor

Is it possible also to remove the library? I see there is the asadmin remove-library command too, however I have no idea how it works if the application is already running and using the library.

Perhaps, on Linux (and Unix generally) library file will be removed, but library will be accessible until server stop/restart. On Windows remove-library command should be failed due to file locking.

@OndroMih
Copy link
Contributor Author

The remove-library command just deletes the JAR on the file system. It could remove the url in the classloader but it’s not necessary. The CL will just ignore the url if the file doesn’t exist. So no need to remove the url from the CL. Classes loaded from the removed JAR will keep being loaded even after the JAR is removed.

@avpinchuk
Copy link
Contributor

The CL will just ignore the url if the file doesn’t exist

If JAR was already opened by CL, CL will not ignore this JAR. Application will load classes and resources from this JAR.

@dmatej
Copy link
Contributor

dmatej commented Sep 24, 2024

We already have this command, so it should probably do more, ie. refuse to remove the file if it is used by any classloader and report that classloader to the user. We can try to do that in another PR, but for this reason I always pushed libraries inside ear/war files, because they have lifecycle while this is quite fragile.

Could it be enough to disable the application and then remove the library? Maybe we can move this to discussions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature A major new user functionality was added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants