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

Improve application-specific libraries loading #24946

Merged

Conversation

avpinchuk
Copy link
Contributor

@avpinchuk avpinchuk commented May 6, 2024

When Jakarta EE applications and modules use shared framework classes (such as utility classes and libraries), the classes can be put in the path for an application-specific class loader rather than in an application or module.

Application libraries are included in the AppLib class loader.

If multiple applications or modules refer to the same libraries, classes in those libraries are automatically shared.

Since these libraries are external to applications and the GlassFish server itself, they can be deleted or replaced with new versions using operating system tools. The result is system dependent.

It is necessary to make the behavior of application libraries the same on all supported platforms.

The proposed solution attempts to achive this by creating temporary snapshots of the application libraries during application startup.

An application library is identified by its URI and some set of basic file attributes (size, lastModifiedTime and fileKey). If a library with the same URI and attributes is already loaded, use it. If not, load it and put in the registry of loaded application libraries for reuse by other applications.

First we try to create a temporary snapshot of the application library:

  1. Open the FileInputStream, get the native open file descriptor from it and read its basic file attributes.
  2. Reading data from the open input stream into temporary file.
  3. Closing the input stream.

Performing these steps ensures that the file attributes match exactly the library content. This gives us some degree of atomicity.

A discrepancy is possible, for example, in the following scenario:

  1. Read basic file attributes.
  2. Replacing a library with an externa process.
  3. Create a temporary snapshot of a library

In this case basic fie attributes will not match the library.

If we are unable to create a temporary snapshot of the library using native descriptor, we fall back to using standard NIO.2 functions.

If we cannot create a snapshot of the library for some reason, we fall back to using the original library.

Last but not least, we close application libraries class loaders and delete snapshots when the GlassFish server is stopped.

@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch 2 times, most recently from 8655697 to e86d740 Compare May 6, 2024 21:27
@avpinchuk
Copy link
Contributor Author

@dmatej, what do you think about the concept itself?

@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch 3 times, most recently from cbf7e0b to f5e04c2 Compare May 19, 2024 21:08
@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch from f5e04c2 to 48a9874 Compare June 10, 2024 20:27
@avpinchuk
Copy link
Contributor Author

Main part is done. Only minor changes need to be made. What you think, @dmatej?

@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch 2 times, most recently from a995ce9 to 5342b80 Compare June 12, 2024 15:11
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch from 5342b80 to c2c3762 Compare June 12, 2024 16:49
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk avpinchuk force-pushed the non-caching-applib-classloader branch from ce38f70 to 21a7ca4 Compare June 13, 2024 15:27
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@dmatej
Copy link
Contributor

dmatej commented Jun 13, 2024

Main part is done. Only minor changes need to be made. What you think, @dmatej?

I will try before the end of the week, now I am struggling with some network issues and I am several days behind my email notifications ...

Method method;
Field field;
try {
if (OS.isWindows()) {
Copy link
Contributor

@dmatej dmatej Jun 13, 2024

Choose a reason for hiding this comment

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

Isn't there already a way how to get these with NIO? Maybe just with newer JDK versions, I am not sure now. I have to check it later ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't there already a way how to get these with NIO? Maybe just with newer JDK versions, I am not sure now. I have to check it later ...

I just need to add logging and do some code cleanup... After that, I will add a detailed description of why the decision was made to use certain platform-dependent means.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that it may change or even be abandoned in the future. How critical is the difference from this? https://www.baeldung.com/java-nio2-file-attribute

Is this the whole explanation?

If we are unable to create a temporary snapshot of the library using native descriptor, we fall back to using standard NIO.2 functions.

Copy link
Contributor Author

@avpinchuk avpinchuk Jun 22, 2024

Choose a reason for hiding this comment

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

The problem is that it may change or even be abandoned in the future. How critical is the difference from this? https://www.baeldung.com/java-nio2-file-attribute

This prevents inconsistency between file attributes and library content in case where library has been replaced with new one after the attributes were read, but before the snapshot was created.

First, I open FileInputStream. This block the file from being replaced or deleted on Windows and saves the contents of the file (inode?) on UNIX until the input stream closes. Then I get the open file descriptor (handle on Windows) and read the attributes from it. Then I copy the open input stream to a temporary file and close input stream. This ensures that the attributes match the file exactly.

If for some reason it is impossible to read attributes using os-dependent functions, we return to using standard NIO.2 functions (as described in the Baeldung article).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it may change or even be abandoned in the future

In this case snandard NIO.2 will be used. Maybe until another solution is found.

Copy link
Contributor Author

@avpinchuk avpinchuk Jun 22, 2024

Choose a reason for hiding this comment

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

I did tests on Temurin 11-22 and some latest Semeru.

I also tried using simple tests how to get the attributes of open files on Windows, Linux, macOS and Solaris(OpenIndiana). For OpenJDK-derived JVMs from 11 to 22 all works now.

Copy link
Contributor

Choose a reason for hiding this comment

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

It still smells to me as some bad practice, but ok, lets try that, if somebody finds it problematic in practice, I hope he will report it. Maybe it would be worth to create an issue for JDK to "open these doors".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still smells to me as some bad practice

Unfortunately, Java doesn't have methods FileInputStream.getAttibutes(), FileChannel.getAttributes(), Files.readAttributes(FileDescriptor fd, ...) or similar. Therefore we are forced to use the "reflection magic".

lets try that, if somebody finds it problematic in practice, I hope he will report it

This should never fail. If "reflection magic" does not work, the standard path will:

            // Fallback to the standard NIO.2 method
            if (attributes == null) {
                attributes = readAttributes(libURI);
            }

In readAttributes() method

            return Files.readAttributes(Path.of(libURI), BasicFileAttributes.class);

Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk
Copy link
Contributor Author

Tested on Ubuntu 22.04.4 LTS, Windows 10 22H2 and macOS 14.5.

@avpinchuk avpinchuk changed the title Non-caching class loader for deployment time libraries Improve application-specific libraries loading Jun 17, 2024
@avpinchuk avpinchuk added this to the 7.0.16 milestone Jun 17, 2024
@avpinchuk avpinchuk added bug Something isn't working enhancement New feature or request code cleaning labels Jun 17, 2024
@avpinchuk avpinchuk self-assigned this Jun 17, 2024
@avpinchuk avpinchuk marked this pull request as ready for review June 17, 2024 21:02
Copy link
Contributor

@hs536 hs536 left a comment

Choose a reason for hiding this comment

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

LGTM. That snapshots is needed to troubleshoot app problems, but I'm not sure if the system's temp directory is the best place for it.

@avpinchuk
Copy link
Contributor Author

Thank you for reply! I thought about it, but I don't know where the right place is. Any suggestions?

@hs536
Copy link
Contributor

hs536 commented Jun 19, 2024

Any suggestions?

Humm... there are several candidates...

  • system tmp dir (now)
  • domain/node root dir: /glassfish/domains/domain1(/snapshots?)
  • domain/node lib dir: /glassfish/domains/domain1/lib(/applibs? /snapshots?)
  • applications dir: ls -ltr /glassfish/domains/domain1/applications(/snapshots? /__internal/snapshots?)

The /glassfish/domains/domain1/lib/applibs directory may not be appropriate since it is where users install external jars.
How about creating a new directory for the snapshots in the same hierarchy as applibs?

  • /glassfish/domains/domain1/lib/snapshots

Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk avpinchuk requested a review from hs536 June 19, 2024 23:10
@arjantijms
Copy link
Contributor

Any suggestions?

Humm... there are several candidates...

  • system tmp dir (now)
  • domain/node root dir: /glassfish/domains/domain1(/snapshots?)
  • domain/node lib dir: /glassfish/domains/domain1/lib(/applibs? /snapshots?)
  • applications dir: ls -ltr /glassfish/domains/domain1/applications(/snapshots? /__internal/snapshots?)

The /glassfish/domains/domain1/lib/applibs directory may not be appropriate since it is where users install external jars. How about creating a new directory for the snapshots in the same hierarchy as applibs?

  • /glassfish/domains/domain1/lib/snapshots

There's cons and pros to system tmp and domain/[somewhere].

System tmp gets cleaned automatically, at some point. The con is that the connection between the usage of the tmp folder and the using application is less clear. Lots of other tmp stuff is put inside the GF folders now, like e.g. all the generated java code for Pages (JSP).

@dmatej
Copy link
Contributor

dmatej commented Jun 22, 2024

To the temporary dir - on linux user can use soft link and link it to the operating system. However I don't remember how it would affect replications in cluster - and also if it should affect them.

@avpinchuk
Copy link
Contributor Author

To the temporary dir - on linux user can use soft link and link it to the operating system. However I don't remember how it would affect replications in cluster - and also if it should affect them.

Snapshots are excluded from replication. Replication of an original libraries left unchanged.

@arjantijms
Copy link
Contributor

LGTM now. If anything needs to be enhanced still, let's do it in a follow-up PR.

@arjantijms arjantijms merged commit bb053a8 into eclipse-ee4j:master Jun 23, 2024
2 checks passed
@dmatej
Copy link
Contributor

dmatej commented Jun 23, 2024

LGTM now. If anything needs to be enhanced still, let's do it in a follow-up PR.

You should wait a bit until we finish the discussion. Now I cannot add my approval and two years later I might argue I did not approve it :D

Btw all tests including TCK I have on my Jenkins passed two hours ago, so I would approve it now. So I do that at least as a comment.

@avpinchuk avpinchuk deleted the non-caching-applib-classloader branch June 29, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code cleaning enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants