Hey guys, catching up. Several questions/comments:
Christian Schneider The jpa container defined in the OSGi spec should make sure to detect when a persistence unit bundle goes away and automatically unpublish the EntityManagerFactory and then close it.
Can you clarify what you mean by "unpublish the EMF"? For use with Aries JPA container-managed JPA, closing the EMF makes sense (I think...), but I'm tripping over what you mean by unpublishing.
Bram Pouwelse I think it would be best to use the ClassLoader from the requestingBundle in the OsgiPersistenceProvider instead of creating a new ClassLoader that is mashing the ClassLoaders of all bundles using Hibernate in one OsgiClassLoader. By using the ClassLoader from the requestingBundle and not using the OsgiClassLoader there is no need track bundle events to cleanup.
As discussed on https://github.com/hibernate/hibernate-orm/pull/958, it's a great thought, but misses some nuances that still necessitate the need for the combined OsgiClassLoader. IIRC, loading resources provided by hibernate-entitymanager/hibernate-core (schemas, etc.) needed this approach. There may have been other reasons, but the specifics escape me. Explicitly setting it up this way is more consistent, rather than requiring the "requesting bundle" to ensure it's importing all packages necessary to make that work...
Christian Schneider I agree that PersistenceUnitInfo.getClassLoader() can only help for container managed JPA. In aries jpa we use this mode. We set it to the classloader of the bundle that contains the persistence.xml. This works for all cases I have seen. Steve Ebersole ...seems like using just the PersistenceUnitInfo#getClassLoader would seem to make sense
This was in regards to container-managed JPA. Originally, the thought here was the same as the comment as above. But, if Aries really is setting the PersistenceUnitInfo.getClassLoader() up that way, maybe we'd be ok. The only piece that worries me here is "the classloader of the bundle that contains the persistence.xml", implying only hibernate-entitymanager, but missing hibernate-core. In theory that sounds ok (since core packages are imported in em), but again, nuances...
Steve Ebersole Here I think the BundleListener is needed. But maybe we should just "take a page" from the e-OSGi spec and simply shutdown the SF/EMF when the requesting-bundle goes away.
This was in regards to unmanaged JPA and Native. +1 from me – this was my original intention.
Christian Schneider For application managed mode I think the ThreadContextClassloader could make sense. If the user knows that he has to set it berfore calling Persistence.createEntityManagerFactory then it should be safe.
-1 from me. My hope was to make this work OOTB as much as possible through automated-bootstrapping and OSGi services, rather than requiring workarounds.
Christian Schneider For OSGi it is always good to provide the user a way to give you the classloader as he then has full control which to use. If you would blindly use the classloader of the class where Persitence.createEntityManagerFactory is called then it may not always be the correct one. Providing a special key name for the classloader to use in the properties that can be given to Persistence.createEntityManagerFactory(java.lang.String persistenceUnitName, java.util.Map properties) would also help a lot.
However, this one makes a lot of sense – +1 to making assumptions, unless overridden explicitly.
Steve Ebersole What I think should happen is that org.hibernate.osgi.OsgiSessionFactoryService and org.hibernate.osgi.OsgiPersistenceProviderService should each create the OsgiClassLoaderService per each call to their #getService calls.
+1, but IIRC this is already fixed. A single OsgiClassLoaderService for the entire hibernate-osgi activation was absolutely incorrect, but was temporarily needed for our TCCL workarounds.
Christian Schneider If you are interested in that approach I could create a prototype.
Christian, admittedly, I've never used WeavingHook or transformers, so I'm a bit lost on that side of the discussion. But if there's something we could be doing that would work well, I'm all for a pull request, if you have time...
Bram Pouwelse One can workaround this issue by importing the org.hibernate.proxy and javassist.util.proxy packages in the bundle bundle containing the entity class. I've also did a quick test changing the org.hibernate.proxy.pojo.javassist.JavassistProxyFactory#buildJavassistProxyFactory method to use a ClassLoader that aggregates two ClassLoaders (the Hibernate ClassLoader and the ClassLoader of the entity class). This resolves the issue and I don't see any ClassLoader issues.
Steve Ebersole, +1 from me on this. We currently instruct users to import those 2 packages, but I never had a chance to really dig into the cause. Am I correct that this still needs done? I haven't dug much into your recent work, but am I assuming correctly that the ClassLoaderService setup would allow this to easily happen? |