Static caching of Persistence providers by bootstrap class can cause Class loading errors in Web Application deploy/redeploy cycle

[glassfishrobot]

javax.persistence.Persistence (henceforth bootstrap class) finds all PersistenceProviders on-first-demand and caches available provider classes statically i.e. in a Set which is declared static.

Consider the following scenario: 1) web application W has packaged libraries of Persistence Provider X in the WEB-INF/lib directory. Agreed that packaging the Persistence Provider libraries in WEB-INF/lib (instead of shared library) is may not be the 'best practice' – but nothing stops that choice.

2) W is deployed in an appserver.

3) Some code in W calls javax.persistence.Persistence.createEntityManagerFactory(). Internally, bootstrap finds X (loaded by ClassLoader L1) and caches it in its internal static map.

4) W is undeployed. So class loader L1 is no more.

5) W is redeployed and javax.persistence.Persistence.createEntityManagerFactory() is called again. The code will find class X in its static set of PersistenceProviders, invoke createEntityManagerFactory on X and because the classloader of X i.e. L1 is no more – all sorts of very long class loading exception stack traces will start flying through people's e-mailboxes.

Suggested solution: a) Given Persistence.createEntityManagerFactory() should be an infrequent operation by user application, one solution is to cache nothing and find list of providers everytime afresh. b) For those who is thinking 'performance will suck if that is done' – alternative is to cache the class names rather than the PersistenceProvider classes themselves.

Environment

Operating System: All Platform: All

Affected Versions

[9.0pe]

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/glassfish/issues/3229
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @yaminikb
• Original Issue Closed By:@glassfishrobot

[glassfishrobot]

@glassfishrobot Commented gfbugbridge said:

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Does server restart solves the problem (at least as a work around)?

[glassfishrobot]

@glassfishrobot Commented ppoddar said: Surely server restart will solve the problem. But that is a rather un-user-friendly workaround – because deploy/redeploy is typical in dev cycle. The nicer workaround, imo, is to place the Persistence Provider libraries in shared library rather than packaging it with enterrpise/web app. However, that model has the limitations at enterprise environment regarding version upgrade of individual enterprise/web apps.

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Downgrading because there is a workaround.

[glassfishrobot]

@glassfishrobot Commented ss141213 said: This bug should be fixed immediately. There are several side effects of this bug in a container environment for applications that use JPA in Java SE style: 1. A provider bundled as part of application 1 can be used by application 2. 2. Different versions of a PersistenceProvider can't be used by two different applications. 3. The set of PersistenceProviders is limited to what is visible to the first application that called Persistence.createEntityManagerFactory.

All these are sort of spec vioaltions. An example of what kind of nightmare stack trace it can lead to can be found at http://www.jboss.com/index.html?module=bb&op=viewtopic&t=110583&postdays=0&postorder=desc&start=0 It's a coincidence that we have a OpenJPA developer filing this bug in GlassFish at the same time as another user complaining about some weired behavior while running JBoss SEAM in GlassFish!

– Sahoo

[glassfishrobot]

@glassfishrobot Commented ppoddar said:

Downgrading because there is a workaround. This decision came as a bit of surprise.

Application deployment/redeployment is a very common usage scenario especially given the newness of JPA, I am sure lots of developers are going to try it out.

The workaround exists – but if an implementation of JPA specification itself suggests workaround rather than solving the core issue – where is that approach going to end?

Any bootstrap class (javax.persistence.Persistence, in this context) caching classes in an environment where multiple classloaders are involved is a simple recipe for disaster.

The good news is both the suggested solution i.e. either not caching anything or just caching class names are simple to achieve.

If you accept, we (BEA) can contribute a revision of javax.persistence.Persistence in a short notice.

[glassfishrobot]

@glassfishrobot Commented mm110999 said: A more "developer friendly" workaround is to put the provider jars higher up in classloader hierarchy. For example in glassfish, the provider jars can be dropped into $

{glassfish.home}

/ lib directory which would make them part of system class loader. This should take care of most of use cases. But, it is still a workaround and would still leave following use cases exposed [1] User wants to bundle different version of same providers with different apps deployed in same container [2] User does not have privileges to drop provider jars higher enough in class loader hierarchy (for example a hosted instance of glassfish) and he is using pattern described in the bug

I agree with ppoddar's suggestion to not use caching as Persistence.createEMF() is an infrequent operation. Lets favor simpler code rather than optimizing for a bottleneck thats not yet proven

Attached is a version of Persistence.java that implements the changes. I think in final implementation, we should try to merge in changes submitted with patch 2814 also.

Now the question is since, Persistence.class ships as part of persistence api, what is the process to release a version of persistence api that has these changes. We are working on the process.

Since there are workaround available, downgrading the bug to P3 to track this it correctly for glassfish release purpose. Which means this bug will be fixed as first available opportunity but definitely before FCS. (Refer to discussion here (https://glassfish.dev.java.net/servlets/ReadMsg?listName=dev&msgNo=3649) on bug priority)

[glassfishrobot]

@glassfishrobot Commented mm110999 said: Created an attachment (id=982) Proposed changes to fix the bug. Please note that the final checkin should merge in changes from patch 2814 also

[glassfishrobot]

@glassfishrobot Commented ppoddar said: Created an attachment (id=989) Combined version that a) does not cahce Providers b) handles errors in further details. Please see more detailed description in the following note.

[glassfishrobot]

@glassfishrobot Commented ppoddar said: The attached version of javax.persistence.Persistence implements following changes:

1. Removes Caching of PersistenceProvider: This version does not statically cache PersistenceProvider classes. This allows multiple providers being visible in different classloding scenarios. Every invocation of createEntityManagerFactory() looks up the list of persistence providers by current thread's context classloader.

2. Produces more detailed error messages: This bootstrap can fail to createEntityManagerFactory() in multiple ways. For example: a) no 'services file' META-INF/services/javax.persistence.PersisteceProvider is available. b) Services file(s) is(are) available, but none of the specified PersistenceProvider classes can be loaded by the current classloader. c) One or more PersistenceProvider class is available but none can create an EntityManagerFactory.

3. Robustness against non-compliant provider: This bootstrap loops through available providers and requests them to create EntityManagerFactory for the named persistence unit. The compliant provider should return null if it can not create a factory for the given unit according to JPA specification contract for PersistenceProvider.createEntityManagerFactory() . This bootstrap is based on nullity of return value and returns as the first provider successufully returns a non-null EntityManagerFactory.

If any of the provider, however, throws exception rather than returning null, the bootstrap breaks. This version catches such exception and continues with rest of the providers.

4. Tracability: A main() method is added to report the visible persistence providers and reasons, for any of the providers failing to be loaded by the context classloader.

5. Ordering There is no defined ordering of providers dictated by JPA specification. As mentioned earlier, Persistence.createEntityManagerFactory() returns on the first successful provider. While this is acceptable but it leaves the question open: In which order the providers are offered a chance to create EntityManagerFactory?

This version ensures that the order is dictated by the classpath ordering of the providers. One could impose lexical ordering based on implementaion class name of the persistence provider, but that will be unfair and inflexible.

[glassfishrobot]

@glassfishrobot Commented mrjdo said: Hi Pinaki,

Thanks for consolidating the two patches. Just a couple of comments.

1. There is a typo that needs to be fixed: public static final String PERSISTENCE_PROVIDER = "javax.persistence.spi.PeristenceProvider";

2. No test cases are provided part of the patch. This code desperately needs test cases for each condition that's being tested for. This might be more obvious if you look at this code: In createEntityManagerFactory, try

{ providers = findAllProviders(); }

catch (IOException exc){}; In findAllProviders, throw new IOException(loader + " can not load any service descriptor as resource [" + serviceName + "]"); So where exactly is this exception message going to be given to the user?

3. No security authentication doPrivileged blocks are used.

4. javadoc needs improvement. Partial javadoc, illegal javadoc For example, *

• The order of */ private static List findAllProviders() throws IOException {

5. The method findAllProviders might find one or more providers but burp error messages to the console.

6. Without seeing the error message, it's hard to tell if simply printing Throwable.toString() will give enough information to be useful.

7. For the message WARNING: One or more PersistenceProviders are not configured properly, it's hard to tell if simply printing TreeMap<String, Throwable>.toString() will be useful.

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Craig,

Thank you for the review. Unfortunately we can't fix the typo in #1 on your list

• it's part of the signature and will require changes to the CTS/TCK as well.

regards, -marina

[glassfishrobot]

@glassfishrobot Commented ppoddar said: 1. Sorry for the typo. Will submit a corrected version with this typo + JavaDoc.

2. Test: what test harness these tests must run? I have few pure JUnit Tests to verify the patch.

So where exactly is this exception message going to be given to the user? If the error is fatal i.e. no provider – a PersistenceException is raised. The message of the exception will contain the stack traces of each individual failures. If the error is non-fatal i.e. one or more providers are found but one or more providers are also unusable – a warning is printed in System.err and processing continues.

Printing the error is console is same as in the original.

3. No Security: Security considerations are not covered by this patch.

4. JavaDoc: will add/correct

5. Yes. It does complain about providers that are specified by name but can not be loaded for one reason or another. It helps rather than being silent.

6. careful reading will reveal that the code does not print TreeMap as error messages. It expands the content of the TreeMap.

7. See (6)

[glassfishrobot]

@glassfishrobot Commented mvatkina said: I'll take a look

[glassfishrobot]

@glassfishrobot Commented mvatkina said: ...

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Please review the attached version that combines (or at least tries to) from all 3 proposals.

Please note that I ended up loading the provider twice in case of failures because I can't put 2 results on the stack without a wrapper around them (or another hack of using an Object[]). And I'm not sure that adding an inner static class is permissible by the signature test. I can probably refactor the duplicate code into a separate method if anybody feels strong about it.

Pinaki, I used your approach to notify about the lack of services at the point where they are being read.

Craig, I added another part of your checks when all providers returned null. The tests attached to issue https://glassfish.dev.java.net/issues/show_bug.cgi?id=2814 produced the following output: test-no-services: [junit] Running javax.persistence.PersistenceTestNoServices [junit] createEntityManagerFactoryNoServices [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.02 sec

test-no-persistence.xml: [junit] Running javax.persistence.PersistenceTestNoPersistenceXML [junit] createEntityManagerFactoryNoPersistenceXML [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.011 sec [junit] Running javax.persistence.PersistenceTest [junit] createEntityManagerFactory [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.018 sec

Is this all that you added?

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Created an attachment (id=1037) The next combined version

[glassfishrobot]

@glassfishrobot Commented mvatkina said: I'll attach the latest version. Added try-catch block around provider.createEntityManagerFactory() and changed message text to a stack trace for all other than a ClassNotFoundException (as suggested by Pinaki and Craig).

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Created an attachment (id=1042) Version that prints stack traces

[glassfishrobot]

@glassfishrobot Commented mvatkina said: Notes:

1. As compared to the latest attached version, I needed to completely restore the declaration of the 'providers' field to be able to pass the signature test.

2. The changes will be pushed to maven when I fix issue with the spec version in the manifest file: https://glassfish.dev.java.net/issues/show_bug.cgi?id=3367

Checking in src/java/javax/persistence/Persistence.java; /cvs/glassfish/persistence-api/src/java/javax/persistence/Persistence.java,v <-- Persistence.java new revision: 1.9; previous revision: 1.8 done

[glassfishrobot]

@glassfishrobot Commented mvatkina said: http://fisheye5.cenqua.com/changelog/glassfish/?cs=MAIN:mvatkina:20070719192834

[glassfishrobot]

@glassfishrobot Commented mvatkina said: The changes has been pushed to maven as version 1.0.2

[glassfishrobot]

@glassfishrobot Commented File: Persistence.java Attached By: mvatkina

[glassfishrobot]

@glassfishrobot Commented File: Persistence.java Attached By: mvatkina

[glassfishrobot]

@glassfishrobot Commented File: Persistence.java Attached By: ppoddar

[glassfishrobot]

@glassfishrobot Commented File: Persistence.java Attached By: mm110999

[glassfishrobot]

@glassfishrobot Commented Was assigned to mvatkina

[glassfishrobot]

@glassfishrobot Commented This issue was imported from java.net JIRA GLASSFISH-3229

[glassfishrobot]

@glassfishrobot Commented Reported by ppoddar

[glassfishrobot]

@glassfishrobot Commented Marked as fixed on Monday, July 23rd 2007, 8:30:25 am