Maintenance Reviews was: [PATCH] Enhance ServiceLoader to understand factory methods

[Jaroslav Tulach]

Eamonn McManus wrote:
> <p>I can act as sponsor for this change.<br>

Excellent, thanks Eamonn.

> Can you say a bit more about the rationale here? I understand why
> SPIs should be defined with abstract classes rather than interfaces,
> but ServiceLoader already supports both classes and interfaces.

I want to provide a compatible addition to existing behaviour. As such, what
used to work can still continue to be used.

> I can 
> see why final classes might be of interest (so you can add new methods
> without fearing a clash with an existing subclass method name), 

One of the things that NetBeans learned in the past is that API for clients
shall be separated from the API for providers to simplify
classes/interfaces evolution. This is not really easy to do with current
ServiceLoader design. TheService class in visible to both, the providers
(obviously) as well as clients (everyone can call
ServiceLoader.load(TheService.class)). This makes TheService class
evolution tricky.

We are usually workarounding the problem just like in case of FileOwnerQuery
(the API for clients) and FileOwnerQueryImplementation (this is the service
class):
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectapi/org/netbeans/api/project/FileOwnerQuery.html
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectapi/org/netbeans/spi/project/FileOwnerQueryImplementation.html
However this is not bulletproof, malicious client code can access
FileOwnerQueryImplementation directly. With the enhanced ServiceLoader this
could be eliminated:

public final class FileOwnerQuery {
  public static FileOwnerQuery create(Implementation impl) {
    return ...;
  }

  public static Project  getOwner(URI uri) { 
      for (FileOwnerQuery q :
ServiceLoader.load(FileOwnerQuery.class).iterator()) {
        // do something with q
      }
  }  

  public interface Implementation { 
    // some methods for the provider
  }
}

Then people providing some implementation could:

public final class MyFileOwnerQueryImpl implements
FileOwnerQuery.Implementation {
  public static getDefault() {
    return FileOwnerQuery.create(new MyFileOwnerQueryImpl());
  }

  // some methods implemented
}
and in META-INF/services/org.nb.FileOwnerQuery have:
com.mymodule.MyFileOwnerQueryImpl.getDefault()

This may not sound like a big improvement and it really is not for the first
version. However, as soon as you find out that the Implementation interface
is completely wrong you can add:

public final class FileOwnerQuery {
  public static FileOwnerQuery create(DifferentImplementation impl) {
    return ...;
  }

  public interface DifferentImplementation { 
    // some other methods for the provider
  }
}

and that is all. The registration stays the same, just people implement the
different class and thus call the other create(...) method. This is much
simpler than with current ServiceLoader. When we faced this situation we
needed to...

http://bits.netbeans.org/dev/javadoc/org-netbeans-api-java-classpath/org/netbeans/spi/java/queries/SourceForBinaryQueryImplementation.html
http://bits.netbeans.org/dev/javadoc/org-netbeans-api-java-classpath/org/netbeans/spi/java/queries/SourceForBinaryQueryImplementation2.html

... create the "2" interface, but that interface needed to extended the "1"
interface, which was quite limiting. In short our NetBeans experience tells
us that static "factory methods" can simplify services evolution and I'd
like to contribute this to OpenJDK.

> and I 
> guess you have in mind that such classes could have their behaviour
> defined via constructor parameters, with each new version adding a new
> constructor; but rather than guessing it would be nice to see an
> example.

Another nice thing is that now it is possible to create "service providers"
without subclassing. For example we have:
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectuiapi/org/netbeans/spi/project/ui/PrivilegedTemplates.html
which is sort of service. Right now every module needs to create own
subclass. With the factory method registration we could define:

public final class PrivilegedTemplatesFactory {
  private PrivilegedTemplatesFactory() {}

  public static PrivilegedTemplates create(String... names) { ... }
}

and then the "service providers" could:

public class SomeImplClass {
  public static String[] myPrivilegedTemplates() { 
    return PrivilegedTemplatesFactory.create("Jaroslav", "Tulach");
  }
}
and register it in META-INF/org.nb.PrivilegedTemplates as
my.module.SomeImplClass.myPrivilegedTemplates()

This could be also useful in certain situations. Are these use-cases
convincing enough? Shall I document them in some OpenJDK
source/documentation file?
-jst

PS: The last example could be made even more generic, if we allowed simple
constants to be arguments to the factory methods, like having
META-INF/org.nb.PrivilegedTemplates with
org.nb.PrivilegedTemplatesFactory.create("Jaroslav", "Tulach")
but that is probably too complicated for discussion, specification and even
implementation, and as far as I remember NetBeans does not need that.


> Jaroslav Tulach wrote:
> <blockquote cite="mid:gda0dl$81n$1 at ger.gmane.org" type="cite">
>   <pre wrap="">Thanks for the clarification, Mark. I've heard about some
>   umbrella JSR for
> each release of the JDK before and I was hoping that my proposed change
> could be covered as part of it. Thanks for confirming that such
> aggregation is possible.
> 
>   </pre>
>   <blockquote type="cite">
>     <pre wrap="">With regard to this particular proposal, as the original
>     author of ... </pre>
>   </blockquote>
>   <pre wrap=""><!---->
> OK, I can promise that I will work hard on polishing my proposal. I can
> improve the code, I can add more documentation, I can write more tests,
> change the new API semantics, etc. I just need a sponsor to tell me what
> to change and where and then take and apply my final patch.
> 
> Thanks in advance for your help.
> -jst
> 
> Dne Friday 17 October 2008 06:24:13 Mark Reinhold napsal(a):
>   </pre>
>   <blockquote type="cite">
>     <blockquote type="cite">
>       <pre wrap="">Date: Fri, 17 Oct 2008 05:56:13 +1000
> From: <a class="moz-txt-link-abbreviated"
>
href="mailto:david.holmes at sun.com">david.holmes at sun.com</a>
> 
> If this is a modification to the API not just a patch for the
> implementation then the short answer is No.
>       </pre>
>     </blockquote>
>     <pre wrap="">Well, not exactly.
> 
>     </pre>
>     <blockquote type="cite">
>       <pre
>      
wrap="">                                     
>            API changes have to be made
> via a JSR.
>       </pre>
>     </blockquote>
>     <pre wrap="">... or in a Maintenance Review of an existing JSR.
> 
> Small API revisions and additions do not require their own JSR; that
> would be ridiculously inefficient.  Instead we roll them up into a
> series of Maintenance Reviews of the previous Platform JSR [1].
> 
> There have already been many small API changes in the JDK 7 codebase.
> Suggested patches for further such changes are welcome, and will be
> discussed and judged on their own technical merit.  All implemented
> changes will, in due course, be aggregated into an appropriate JCP
> Maintenance Review.
> 
> With regard to this particular proposal, as the original author of
> the class in question I suppose I should have an opinion about it,
> but that's a topic for a different message.
> 
> - Mark
> 
> 
> [1]
> 
>     </pre>
>   </blockquote>
>   <pre wrap=""><!----><a class="moz-txt-link-freetext"
>  
href="http://weblogs.java.net/blog/mreinhold/archive/2006/03/mustang_mainten.html">http://weblogs.java.net/blog/mreinhold/archive/2006/03/mustang_mainten.html</a>
> 
> 
>   </pre>
> </blockquote>
> </body>
> </html>