This cache is per-ClassLoaderService. My first question is where we have a real use case of ClassLoaderService being re-used? This would essentially mean we have a BootstrapServiceRegistry used in multiple SessionFactory constructions - which is certainly legal, though decidedly not the norm. Given the normal usage, I'm not sure how multiple calls would ever use different security managers. Since there is no test where this shows as a real (not simply theoretical) problem it is hard to say Honestly I do not remember why I cached them. Maybe it was as simple as not wanting to re-load the ServiceLoader. But either way, it would absolutely be wrong to load a ServiceLoader twice. The code as-is enforces that. Removing the caching makes that an implicit requirement that callers would only ever load a given service once; but in my experience making a requirement implicit never goes well. So another option might be to instead cache the service contracts we have loaded and use those as a check. This would work Lot of hypothetical reasoning here. A real test case would help sort some of that. But given what real tests might show, another option might be to instead cache the service contracts we have loaded and use those as a check. E.g.
public <S> Collection<S> loadJavaServices(Class<S> serviceContract) {
if ( loadedServiceContracts.contains( serviceContract ) ) {
throw new ...
}
final ServiceLoader<S> serviceLoader = ServiceLoader.load( serviceContract, getAggregatedClassLoader() );
final LinkedHashSet<S> services = new LinkedHashSet<S>();
for ( S service : serviceLoader ) {
services.add( service );
}
return services;
}
|