ServiceLoader in the JDK

[Paul Sandoz]

On May 23, 2012, at 8:29 PM, Alan Bateman wrote:

> On 23/05/2012 17:31, Paul Sandoz wrote:
>> Hi,
>> 
>> Here is some proposed changes in the JDK related to service loader (not tested yet! just want some easily feedback):
>> 
>>   http://cr.openjdk.java.net/~psandoz/jigsaw/jdk-services/webrev/
>> 
>> I have attempted where possible to tidy things up to avoid an explicit reference to a class loader for common patterns hence the addition of the methods ServiceLoader.loadFromClass, ServiceLoader.loadFromSystem and ServiceLoader.loadFromCaller (when in module mode most ServiceLoader.load* method do the equivalent of the latter). The names could be better.
> I agree the names are problematic, I'm also not sure that we really need them either.
> 
> loadFromClass(c) is specified to work like load(c,c.getClassLoader()) but it works differently when working with modules?
> 

Yeah it uses the same hack as for load(Class, ClassLoader)


> loadFromSystem is specified to use the system class loader so will be confusing when running with modules. For the 10 or so cases in the JDK then an alternative push the check back to the use sites.
> 

Yes, it was attempting to encapsulate "if (Platform.isModuleMode()) { // do this } else { // do that }"


> I'm also not sure about loadFromCaller. Determining the caller is expensive and at least for the usages in the JDK then load(Class,ClassLoader) should be okay.
> 
> One other thing about ServiceLoader is that we have to put in a few hacks (to use the caller loader) in order to keep it working with existing code. Once all usages are cleaned then this of course can be revisited.

I knew what was done in load(Class, ClassLoader) was a hack but i was laboring under the misapprehension that the general use of the caller loader was not.

So it is clear that all the hacks should go away and in module mode the only call that really makes sense is load(Class, ClassLoader) with the appropriate ClassLoader of the module.

So this:

  ServiceLoader.load(HttpServerProvider.class, ClassLoader.getSystemClassLoader());

Could change to:

  ServiceLoader.load(HttpServerProvider.class, Platform.isModuleMode() 
        ? HttpServerProvider.class.getClassLoader() 
        : ClassLoader.getSystemClassLoader());


The choice of null or non null for say Object.class.getClassLoader() is clearly going to affect things here for services in the base module. So this:

  ServiceLoader.load(FileSystemProvider.class, ClassLoader.getSystemClassLoader());
  
changes to:

  ServiceLoader.load(FileSystemProvider.class, Platform.isModuleMode() 
        ? Plaform.getBaseModuleLoader() // is this correct?
        : ClassLoader.getSystemClassLoader());

rather than:

  ServiceLoader.load(FileSystemProvider.class, Platform.isModuleMode() 
        ? FileSystemProvider.class.getClassLoader()
        : ClassLoader.getSystemClassLoader());

which would be consistent with the previous example.

A helper method on Platform could hide this:

  ClassLoader getModuleClassLoaderOrFallback(Class c, ClassLoader fallback) {
    if (Plaform.isModuleMode()) {
      ClassLoader cl = c.getClassLoader();
      return (cl == null) ? getBaseModuleLoader() : cl;
    } else {
      return fallback;
    }
  }


Perhaps we need to introduce an annotation on methods ServiceLoader.load(Class ) and ServiceLoader.loadInstalled(Class ). When compiling in modular mode optional lint like warnings could be given. If such annotations are retained at runtime it should be possible to detect on execution.


> Anyway, I scanned the rest of the JDK patch. One thing is that we'll need to work out spec/javadoc changes for many of these cases.
> 

Yes.


> I would suggest ignoring JMX for now as it will require a special update.
> 
> JDI is specified to use Connector and TransportService providers using the defining loader of the Connector and TransportService types so I don't think it requires any changes.
> 

OK.


> Phil may be able to say something about the rendering engine code but I think this is just for switching between rendering engines and that they are will all be loaded by the module loader for the desktop module.
> 

OK.


> Mandy will likely have an opinion on jconsole but my take is that it's mostly replaced by VisualVM now and maybe we don't do anything and run just it with -Xmode:legacy.
> 
> Having dump-config include the services configuration is a good idea.
> 

I will send a specific patch to the list for this.

Paul.