HHH-13046 ClassLoaderService in JCacheRegionFactory #9352
Conversation
This enables the use of caching in scenarios where a non-standard class loader is registered using the service registry. Unfortunately, since the service registry lifecycle is tied to the lifecycle of the SessionFactory, the required initialization happens too late for some of the test cases to "just work". To keep compatibility with the specs from the test suite, some hackery is required: the JCacheRegionFactory has to hold on to some defaults from before being set up.
milgner
force-pushed
the
feature/cache-class-loader
branch
from
932bcfc to
6d9feba
Compare
There was a problem hiding this comment.
It seems to me that most changes in the JCacheRegionFactory were done for the sake of ensuring tests can continue functioning as they are. I would prefer if we rather change tests to match the "real world" usage by e.g. providing the pre-built CacheManager via the org.hibernate.cache.jcache.ConfigSettings#CACHE_MANAGER property.
| final ClassLoaderService classLoaderService = settings.getServiceRegistry() | ||
| .getService( ClassLoaderService.class ); | ||
| return (classLoaderService == null) ? null : | ||
| classLoaderService.workWithClassLoader( classLoader -> classLoader ); |
There was a problem hiding this comment.
| final ClassLoaderService classLoaderService = settings.getServiceRegistry() | |
| .getService( ClassLoaderService.class ); | |
| return (classLoaderService == null) ? null : | |
| classLoaderService.workWithClassLoader( classLoader -> classLoader ); | |
| return settings.getServiceRegistry() | |
| .requireService( ClassLoaderService.class ) | |
| .workWithClassLoader( classLoader -> classLoader ); |
| } | ||
|
|
||
| protected ClassLoader getClassLoader(CachingProvider cachingProvider, ClassLoader serviceClassLoader) { | ||
| return (serviceClassLoader != null) ? serviceClassLoader : cachingProvider.getDefaultClassLoader(); |
There was a problem hiding this comment.
| } | |
| protected ClassLoader getClassLoader(CachingProvider cachingProvider, ClassLoader serviceClassLoader) { | |
| return (serviceClassLoader != null) ? serviceClassLoader : cachingProvider.getDefaultClassLoader(); |
| final ClassLoader serviceClassLoader = getServiceClassLoader( settings ); | ||
| if ( serviceClassLoader != null ) { | ||
| preInitializedCacheManager = JCacheHelper.locateStandardCacheManager(); | ||
| oldJsr107CacheClassLoader = Caching.getDefaultClassLoader(); | ||
| Caching.setDefaultClassLoader( serviceClassLoader ); | ||
| } |
There was a problem hiding this comment.
IMO, the JCacheHelper class should rather be deprecated and/or moved to the test sources than being used explicitly here.
| final ClassLoader serviceClassLoader = getServiceClassLoader( settings ); | |
| if ( serviceClassLoader != null ) { | |
| preInitializedCacheManager = JCacheHelper.locateStandardCacheManager(); | |
| oldJsr107CacheClassLoader = Caching.getDefaultClassLoader(); | |
| Caching.setDefaultClassLoader( serviceClassLoader ); | |
| } | |
| final ClassLoader serviceClassLoader = getServiceClassLoader( settings ); | |
| oldJsr107CacheClassLoader = Caching.getDefaultClassLoader(); | |
| Caching.setDefaultClassLoader( serviceClassLoader ); |
| final ClassLoader classLoader = getClassLoader( cachingProvider, serviceClassLoader ); | ||
| if ( cacheManagerUri != null ) { | ||
| cacheManager = cachingProvider.getCacheManager( cacheManagerUri, getClassLoader( cachingProvider ) ); | ||
| cacheManager = cachingProvider.getCacheManager( cacheManagerUri, classLoader ); |
There was a problem hiding this comment.
| cacheManager = cachingProvider.getCacheManager( cacheManagerUri, classLoader ); | |
| cacheManager = cachingProvider.getCacheManager( cacheManagerUri, serviceClassLoader ); |
| final CachingProvider cachingProvider = getCachingProvider( properties, serviceClassLoader ); | ||
| final CacheManager cacheManager; | ||
| final URI cacheManagerUri = getUri( settings, properties ); | ||
| final ClassLoader classLoader = getClassLoader( cachingProvider, serviceClassLoader ); |
There was a problem hiding this comment.
| final ClassLoader classLoader = getClassLoader( cachingProvider, serviceClassLoader ); |
| } | ||
| else { | ||
| cacheManager = cachingProvider.getCacheManager( cachingProvider.getDefaultURI(), getClassLoader( cachingProvider ) ); | ||
| cacheManager = cachingProvider.getCacheManager( cachingProvider.getDefaultURI(), classLoader ); |
There was a problem hiding this comment.
| cacheManager = cachingProvider.getCacheManager( cachingProvider.getDefaultURI(), classLoader ); | |
| cacheManager = cachingProvider.getCacheManager( cachingProvider.getDefaultURI(), serviceClassLoader ); |
| // in case caches were already set up beforehand with a different configuration | ||
| private volatile CacheManager preInitializedCacheManager; |
There was a problem hiding this comment.
| // in case caches were already set up beforehand with a different configuration | |
| private volatile CacheManager preInitializedCacheManager; |
| if ( preInitializedCacheManager != null ) { | ||
| final Cache<Object, Object> cacheFromBefore = preInitializedCacheManager.getCache( | ||
| qualifiedRegionName ); | ||
| if ( cacheFromBefore != null ) { | ||
| return cacheFromBefore; | ||
| } | ||
| } |
There was a problem hiding this comment.
| if ( preInitializedCacheManager != null ) { | |
| final Cache<Object, Object> cacheFromBefore = preInitializedCacheManager.getCache( | |
| qualifiedRegionName ); | |
| if ( cacheFromBefore != null ) { | |
| return cacheFromBefore; | |
| } | |
| } |
| return cacheManager.getCache( qualifiedRegionName ) != null || | ||
| (preInitializedCacheManager != null && | ||
| preInitializedCacheManager.getCache( qualifiedRegionName ) != null); |
There was a problem hiding this comment.
| return cacheManager.getCache( qualifiedRegionName ) != null || | |
| (preInitializedCacheManager != null && | |
| preInitializedCacheManager.getCache( qualifiedRegionName ) != null); | |
| return cacheManager.getCache( qualifiedRegionName ) != null; |
|
|
||
| // need to save the classloader from Caching.getDefaultClassLoader to reset it | ||
| // when this service is released again. | ||
| private volatile ClassLoader oldJsr107CacheClassLoader; |
There was a problem hiding this comment.
Is it really necessary to keep around the old class loader? Can't we just swap the class loader when retrieving/looking-up the CacheManager?
|
Hello! I'm commenting on this PR because I've been fighting for 3-4 days with Could be related to what are you guys here solving? Or if you can check if after this change the 2nd level cache with jcache works fine on a native build. Thanks |
|
Hello @milgner , it seems Christian's comments haven't been addressed... Do you intend to resume your work? Otherwise we might have to close the PR and I'd hate to waste that effort :/ |
|
As I currently have some free time on my hands, I'll give this another shot in the next couple of days. I'm also not very happy about the initial approach and will have to spend some more time familiarising myself with the codebase and reviewing the comments. |
|
That's awesome, looking forward to it @milgner . Please reach out on Zulip if you need to: https://hibernate.zulipchat.com/#narrow/channel/132094-hibernate-orm-dev |
Addresses https://hibernate.atlassian.net/browse/HHH-13046 and enables the use of caching in scenarios where a non-standard class loader is registered using the service registry.
Unfortunately, since the service registry lifecycle is tied to the lifecycle of the SessionFactory, the required initialization happens too late for some of the test cases to "just work".
To keep compatibility with the specs from the test suite, some hackery is required: the JCacheRegionFactory has to hold on to some defaults from before being set up.
Note that while I did import the code style into IntelliJ, it seems like not all aspects were automatically resolved as I saw quite a few changes when running a full auto-format over the files. Instead I tried to manually match the existing code style wherever possible.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.