[9] Review request JDK-8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

Georgiy Rakov georgiy.rakov at oracle.com
Thu Oct 1 11:17:51 UTC 2015 

On 30.09.2015 18:08, Miroslav Kos wrote:
>
>
> On 30/09/15 16:20, Deva Sagar wrote:
>> Hi Georgiy,
>>
>> I think both Miran and I are referring to the mention of 
>> SAAJMetaFactory in the bullet list about the public newInstance() 
>> method. The Javadoc of the SAAJMetaFactory class still describes it 
>> as an SPI and it is accessible from package-info. This is no 
>> different from the current public Javadocs.
>>
>> Thanks,
>> Deva
>>
>>
>>> On Sep 30, 2015, at 9:16 AM, Georgiy Rakov <georgiy.rakov at oracle.com 
>>> <mailto:georgiy.rakov at oracle.com>> wrote:
>>>
>>>
>>>
>>> On 29.09.2015 17:43, Deva Sagar wrote:
>>>> Hi Miran,
>>>>
>>>> Sorry for the delay in responding to this - I was busy with WLS 
>>>> release close down, and now I have moved to a different job in the 
>>>> cloud organization outside of WLS and web services.
>>>>
>>>> Regarding your question in #3 - I think Georgiy made a good 
>>>> observation about SAAJMetaFactory - making a public newInstance 
>>>> method as you suggest would make it agree with the package-info, 
>>>> but does not correspond to the API specification. The package-info 
>>>> in Java SE 8 API docs says "SAAJMetaFactory is a service provider 
>>>> interface. There are no public methods on this class.” If we put in 
>>>> a public newInstance() method, it does not agree with this 
>>>> statement in API Javadoc. It might be better in my opinion to 
>>>> remove SAAJMetaFactory mention from package-info.java unless there 
>>>> is a good reason to keep it.
>>>>
>>> Hello,
>>>
>>> according to my understanding the idea of SAAJMetaFactory is for SPI 
>>> developer to subclass it and thus to create a custom SAAJMetaFactory 
>>> implementation.
> Yes, it's current state - the only description of lookup/instantiation 
> user SAAJMetaFactory is statement that it is service provider 
> interaface (stays unchanged): 
> https://docs.oracle.com/javase/8/docs/api/javax/xml/soap/SAAJMetaFactory.html
> Service loader is the only specification here.
>>> Afterwards user should configure SAAJ API in order it could use that 
>>> custom implementation.
> User configuration controls discovery of implementation when used 
> newInstance method on other factories (MessageFactory, SOAPFactory, 
> SOAPConnectionFactory). If no implementation found, the fallback is 
> user (or default) SAAJMetaFactory.newXXXFactory()
>>> But if SAAJMetaFactory mention from package-info.java is removed 
>>> there will be actually no spec defining how to configure SAAJ API 
>>> for using custom SAAJMetaFactory, or do I miss spec defining it?
> No, there is still statement about service provider interface.
I believe then some link or hint should given in order it would be clear 
that ServiceLoader mechanism is used as a lookup procedure of 
SAAJMetaFactory implementation. Because for inexperienced user it's far 
from obvious that mentioning SPI means that ServiceLoader is used to 
discover SAAJMetaFactory implementation (even for me it wasn't :) ).

Besides, do I understand correctly that other steps of lookup procedure 
will still be used by JDK in order to discover SAAJMetaFactory 
implementation? If this is true I don't think it's a good idea to hide 
them by not displaying in spec, since external Java SE implementers like 
IBM and others are required to implement just those steps which are 
provided by spec, as well as Java SE compatible application is required 
to use just those steps which are in spec. So if an Oracle JDK user 
writes an application using this "hidden" feature, which he is aware of 
for some reason, there is no guarantee that such application will work 
fine with an external Java SE implementation. However I understand that 
removing this from spec simplifies SAAJ API I'm not sure that this 
simplifying is worthwhile for the reasons presented above.

Thanks,
Georgiy.
>>> BTW: from this perspective referring to SAAJMetaFactory newInstance 
>>> method in spec is not relevant since it's SAAJ API implementation 
>>> internal machinery.
> Yes, it is internal.
>
> Actually I thing SAAJ API is pretty complicated already, so not adding 
> another spec for SAAJMetaFactory discovery is good idea ...
>
> Thanks
> Miran
>
>
>>>
>>> So I believe if SAAJMetaFactory mention from package-info.java is 
>>> removed the process of SAAJMetaFactory instantiation should be 
>>> described somehow in another place and without mentioning 
>>> newInstance method. Maybe it's worth rather modifying than removing 
>>> this mentioning in package-info.java?
>>>
>>> Thanks,
>>> Georgiy.
>>>
>>>> The rest of your changes look ok to me. Also cc’ing Chen for any 
>>>> additional comments
>>>>
>>>> Deva
>>>>
>>>>
>>>>
>>>>> On Sep 29, 2015, at 9:45 AM, Miroslav Kos 
>>>>> <miroslav.kos at oracle.com> wrote:
>>>>>
>>>>> On 25/09/15 20:24, Georgiy Rakov wrote:
>>>>>> Hello Miroslav,
>>>>>>
>>>>>> sorry for delay, was busy due to deadlines.
>>>>>>
>>>>>> I think it's just spec I am to review. Some comments for the 
>>>>>> moment please see below:
>>>>>>
>>>>>> 1. package-info.java:
>>>>>>
>>>>>>       *  <li>Checks if a system property with the same name as the factory class is set (/_*i.e.*_/
>>>>>>       *  {@code javax.xml.soap.SOAPFactory}).
>>>>>>
>>>>>> I guess "e.g." is meant, that is "for example"?
>>>>> Sure, thanks.
>>>>>>
>>>>>> 2. SAAJMetaFactory.java:
>>>>>>
>>>>>> Following is specified normatively:
>>>>>>    62      * <p>Property {@code javax.xml.soap.MetaFactory} used in previous
>>>>>>    63      * specifications (up to 1.3) is still supported, but it is strongly recommended to migrate to new property
>>>>>>    64      * {@code javax.xml.soap.SAAJMetaFactory}.
>>>>>> while in package-info.java: the same idea is specified as API 
>>>>>> note that is non-normatively:
>>>>>>   * @apiNote
>>>>>>   * <p>For {@link javax.xml.soap.SAAJMetaFactory}, property {@code javax.xml.soap.MetaFactory} used in previous
>>>>>>   * specifications (up to 1.3) is still supported, but it is strongly recommended to migrate to new property
>>>>>>   * {@code javax.xml.soap.SAAJMetaFactory}.
>>>>>>   */
>>>>>> I believe this idea should be specified everywhere the same way 
>>>>>> that is either normatively or non-normatively. If it's to be 
>>>>>> defined normatively more details should be specified I believe.
>>>>> Made non-normative.
>>>>>>
>>>>>> 3. SAAJMetaFactory.getInstance method is package private. So:
>>>>>> - This method is not supposed to be used by users, is it?
>>>>>> - The javadoc of this method isn't actually a public 
>>>>>> specification. Thus Oracle claims nothing by it and external Java 
>>>>>> SE implementers are not required to follow this JavaDoc, that is 
>>>>>> this JavaDoc is intended to be used internally only, isn't it?
>>>>>> I understand that this was before, but It looks very strange. Is 
>>>>>> this really not a mistake? BTW: this method and spec are not 
>>>>>> visible in API JavaDoc 
>>>>>> <https://docs.oracle.com/javase/8/docs/api/javax/xml/soap/SAAJMetaFactory.html>.
>>>>>>
>>>>>> It looks like a mistake moreover because public specification 
>>>>>> defined in package-info.java being reviewed refers to it:
>>>>>>   * There are several factories defined in the SAAJ API to discover and load specific implementation:
>>>>>>   *
>>>>>>   * <ul>
>>>>>>   *     <li>{@link javax.xml.soap.SOAPFactory}
>>>>>>   *     <li>{@link javax.xml.soap.MessageFactory}
>>>>>>   *     <li>{@link javax.xml.soap.SOAPConnectionFactory}
>>>>>>   /_** <li>{@link javax.xml.soap.SAAJMetaFactory}*_/
>>>>>>   * </ul>
>>>>>>   *
>>>>>>   * These factories define {@code/_*newInstance()*_/} method which uses a common lookup procedure to determine
>>>>>>   * the implementation class:
>>>>> It's good catch - it looks like error. An easy fix would be change 
>>>>> the method to newInstance and make it public - Deva, would you agree?
>>>>> http://cr.openjdk.java.net/~mkos/8131334/specdiff.04/index.html
>>>>>
>>>>> Anyway those are changes to API - should I withdraw 
>>>>> http://ccc.us.oracle.com/8131334 and create new one?
>>>>>
>>>>> Thanks
>>>>> Miran
>>>>>
>>>>>
>>>>>
>>>>>> Thank you,
>>>>>> Goergiy.
>>>>>>
>>>>>> On 25.09.2015 16:34, Miroslav Kos wrote:
>>>>>>> Ping ...
>>>>>>>
>>>>>>> On 11/09/15 17:57, Miroslav Kos wrote:
>>>>>>>> Hi again,
>>>>>>>> would somebody find a time to review?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Miran
>>>>>>>>
>>>>>>>> On 20/08/15 16:17, Miroslav Kos wrote:
>>>>>>>>> Hi everybody,
>>>>>>>>>
>>>>>>>>> I am sending changes for review for
>>>>>>>>> JDK-8131334: SAAJ Plugability Layer: using 
>>>>>>>>> java.util.ServiceLoader
>>>>>>>>>
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
>>>>>>>>> webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/
>>>>>>>>>
>>>>>>>>> It's about migrating to standard java.util.ServiceLoader. This 
>>>>>>>>> part of service discovery was implemented previously "own" 
>>>>>>>>> way. There are some changes in javadoc and implementation has 
>>>>>>>>> been refactored in order to use same code as in JAX-WS and JAX-B.
>>>>>>>>>
>>>>>>>>> Testing - I run JAX-WS unit tests (JAX-WS standalone repo), 
>>>>>>>>> JCK9 + new tests specificaly developed for testing service 
>>>>>>>>> discovery in SAAJ.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Miran
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the core-libs-dev mailing list