8182482: Module System spec updates

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

8182482: Module System spec updates

Alan Bateman
We have two javadoc/spec updates that I'd like to get into the JDK 9
Initial Release Candidate that is scheduled for this week.

The spec updates are for two issues:

1. ServiceLoader: The API spec has been updated significantly to support
modules but it needs another round of update to do clean-up to get it
more readable and consistent, and also to align it with the JLS.  Most
of reorganization and re-wording has been proposed by Alex. Joe Darcy
has also proposed a few adjustments.

2. Upgradable modules aren't specified anywhere. Java SE will designate
a number of standard modules as upgradeable but we don't have anywhere
in the docs to link to that or describe how the upgraded versions are
used in preference to the modules built into the environment.

The webrev with the proposed (docs only, no implementation) changes is here:
   http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html

The ServiceLoader diffs are hard to read. It might be easier to read the
generated javadoc:
http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Remi Forax
Hi Alan,
the doc is clearly better.

Can you change
  A service is a single type, usually an interface or abstract class. A concrete class can be used, but this is not recommended.
to
  A service is a single type, usually an interface. An abstract class or a concrete class can be used, but this is not recommended.

An abstract class has a constructor so you can observe when a service implementation is instantiated by the ServiceLoader, which may make the code dependent of the loading order of the service implementations. So using an abstract class for a service implementation is also a bad idea.

regards,
Rémi  

----- Mail original -----
> De: "Alan Bateman" <[hidden email]>
> À: "jigsaw-dev" <[hidden email]>
> Envoyé: Mardi 20 Juin 2017 12:20:16
> Objet: 8182482: Module System spec updates

> We have two javadoc/spec updates that I'd like to get into the JDK 9
> Initial Release Candidate that is scheduled for this week.
>
> The spec updates are for two issues:
>
> 1. ServiceLoader: The API spec has been updated significantly to support
> modules but it needs another round of update to do clean-up to get it
> more readable and consistent, and also to align it with the JLS.  Most
> of reorganization and re-wording has been proposed by Alex. Joe Darcy
> has also proposed a few adjustments.
>
> 2. Upgradable modules aren't specified anywhere. Java SE will designate
> a number of standard modules as upgradeable but we don't have anywhere
> in the docs to link to that or describe how the upgraded versions are
> used in preference to the modules built into the environment.
>
> The webrev with the proposed (docs only, no implementation) changes is here:
>   http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the
> generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html
>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alan Bateman
On 20/06/2017 11:39, Remi Forax wrote:

> Hi Alan,
> the doc is clearly better.
>
> Can you change
>    A service is a single type, usually an interface or abstract class. A concrete class can be used, but this is not recommended.
> to
>    A service is a single type, usually an interface. An abstract class or a concrete class can be used, but this is not recommended.
>
> An abstract class has a constructor so you can observe when a service implementation is instantiated by the ServiceLoader, which may make the code dependent of the loading order of the service implementations. So using an abstract class for a service implementation is also a bad idea.
>
There shouldn't be any issue using an abstract class as the service.
Sure, most developers will choose an interface but we have many examples
where the service type is an abstract class (and there are corner cases
with security where enforcement isn't possible when using an interface).

The service provider can be abstract class too but only when it defines
a public static provider method.

I don't understand your point about an abstract class having a
constructor. If it doesn't a static provider method then it won't
compile as a module (it won't run either as it can't be instantiated).

-Alan



Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

mark.reinhold
In reply to this post by Alan Bateman
2017/6/20 12:20:16 +0200, [hidden email]:
> ...
>
> The webrev with the proposed (docs only, no implementation) changes is here:
>    http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the
> generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html

Looks good -- very good, in fact!  Thanks for rewriting this.

- Mark
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Rony G. Flatscher
In reply to this post by Alan Bateman
Just a side-note: when printing with the latest Firefox the headers cover part of the text, which
makes printing of such Javadocs somewhat useless. Not sure where to report this, so posting it here.

The printout of the generated javadoc is enclosed or if the attachment gets stripped can be fetched
from this location:
<https://www.dropbox.com/s/kzp4wfyl13peccf/ServiceLoader%20%28Java%20SE%209%20%26%20JDK%209%20%5Bad-hoc%20build%5D%29.pdf?dl=0>

---rony

On 20.06.2017 12:20, Alan Bateman wrote:

> We have two javadoc/spec updates that I'd like to get into the JDK 9 Initial Release Candidate
> that is scheduled for this week.
>
> The spec updates are for two issues:
>
> 1. ServiceLoader: The API spec has been updated significantly to support modules but it needs
> another round of update to do clean-up to get it more readable and consistent, and also to align
> it with the JLS.  Most of reorganization and re-wording has been proposed by Alex. Joe Darcy has
> also proposed a few adjustments.
>
> 2. Upgradable modules aren't specified anywhere. Java SE will designate a number of standard
> modules as upgradeable but we don't have anywhere in the docs to link to that or describe how the
> upgraded versions are used in preference to the modules built into the environment.
>
> The webrev with the proposed (docs only, no implementation) changes is here:
>   http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Remi Forax
In reply to this post by Alan Bateman
----- Mail original -----
> De: "Alan Bateman" <[hidden email]>
> À: "Remi Forax" <[hidden email]>
> Cc: "jigsaw-dev" <[hidden email]>
> Envoyé: Mardi 20 Juin 2017 13:30:20
> Objet: Re: 8182482: Module System spec updates

> On 20/06/2017 11:39, Remi Forax wrote:
>> Hi Alan,
>> the doc is clearly better.
>>
>> Can you change
>>    A service is a single type, usually an interface or abstract class. A concrete
>>    class can be used, but this is not recommended.
>> to
>>    A service is a single type, usually an interface. An abstract class or a
>>    concrete class can be used, but this is not recommended.
>>
>> An abstract class has a constructor so you can observe when a service
>> implementation is instantiated by the ServiceLoader, which may make the code
>> dependent of the loading order of the service implementations. So using an
>> abstract class for a service implementation is also a bad idea.
>>
> There shouldn't be any issue using an abstract class as the service.
> Sure, most developers will choose an interface but we have many examples
> where the service type is an abstract class (and there are corner cases
> with security where enforcement isn't possible when using an interface).

Abstract classes are evil when the abstract class and the concrete class are not maintained by the same people,
if the code of the abstract class change for whatever reason, it will break the subclasses. It's just bad design.

And the code that enforce security do not have to be in the abstract class constructor, debugging a constructor in general is harder than any other methods because the object is partially initialized. When you are executing the constructor of an abstract class the object corresponding to this is not fully initialized. You can always 'layer up' and use factories instead.
 
>
> The service provider can be abstract class too but only when it defines
> a public static provider method.

yes, here, it's less problematic because in that case the abstract class and all subclasses will be maintained together.

>
> I don't understand your point about an abstract class having a
> constructor. If it doesn't a static provider method then it won't
> compile as a module (it won't run either as it can't be instantiated).

ok, let's focus on abstract class defining a service.

>
> -Alan

Rémi
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Mandy Chung
In reply to this post by Alan Bateman

> On Jun 20, 2017, at 3:20 AM, Alan Bateman <[hidden email]> wrote:
>
> The webrev with the proposed (docs only, no implementation) changes is here:
>  http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html
>

Looks good.

Mandy

Reply | Threaded
Open this post in threaded view
|

RE: 8182482: Module System spec updates

Uwe Schindler
In reply to this post by Alan Bateman
Hey Alan,

very nice. Reads good.

Although not in this patch, I most like the warning about the ServiceLoader#load(Class) method, context classloader, and caching JVM wide! This is also a great improvement in comparison to Java 8! We currently refactor Lucene because of this context classloader stupidness!

Uwe

-----
Uwe Schindler
[hidden email]
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/

> -----Original Message-----
> From: jigsaw-dev [mailto:[hidden email]] On Behalf
> Of Alan Bateman
> Sent: Tuesday, June 20, 2017 12:20 PM
> To: jigsaw-dev <[hidden email]>
> Subject: 8182482: Module System spec updates
>
> We have two javadoc/spec updates that I'd like to get into the JDK 9
> Initial Release Candidate that is scheduled for this week.
>
> The spec updates are for two issues:
>
> 1. ServiceLoader: The API spec has been updated significantly to support
> modules but it needs another round of update to do clean-up to get it
> more readable and consistent, and also to align it with the JLS.  Most
> of reorganization and re-wording has been proposed by Alex. Joe Darcy
> has also proposed a few adjustments.
>
> 2. Upgradable modules aren't specified anywhere. Java SE will designate
> a number of standard modules as upgradeable but we don't have anywhere
> in the docs to link to that or describe how the upgraded versions are
> used in preference to the modules built into the environment.
>
> The webrev with the proposed (docs only, no implementation) changes is
> here:
>    http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the
> generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.ht
> ml
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alan Bateman
In reply to this post by Rony G. Flatscher
On 20/06/2017 14:02, Rony G. Flatscher wrote:
> Just a side-note: when printing with the latest Firefox the headers cover part of the text, which
> makes printing of such Javadocs somewhat useless. Not sure where to report this, so posting it here.
>
> The printout of the generated javadoc is enclosed or if the attachment gets stripped can be fetched
> from this location:
> <https://www.dropbox.com/s/kzp4wfyl13peccf/ServiceLoader%20%28Java%20SE%209%20%26%20JDK%209%20%5Bad-hoc%20build%5D%29.pdf?dl=0>
>
I'm not 100% sure but it looks it hasn't read the CSS, maybe it's
security setting that prevents it reading "../../stylesheet.css" ?

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alex Buckley
In reply to this post by Remi Forax
Hi Remi,

On 6/20/2017 6:29 AM, [hidden email] wrote:
> ok, let's focus on abstract class defining a service.

I would be happy for the "Designing services" section to give more
advice about the tradeoffs between an interface and an abstract class.
Two sentences, written in a style that leads a junior developer but does
not judge them if they don't follow the advice. Can you write it? :-

-----
A service is a single type, usually an interface or abstract class.
***REMI'S TEXT HERE*** A concrete class can be used, but this is not
recommended. The type may have any accessibility.

The methods of a service are highly ...
-----

Alex

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Rony G. Flatscher
In reply to this post by Alan Bateman
Maybe a few little things:

  * in the Errors section, reason # 4 states:

        The service provider class file has more than one public static no-args method named
        "provider".

    There could be no more than one public static no-args method named "provider" in a class file,
    so this error reason should not be possible?

  * in the "stream" (method-detail) description, second paragraph, second sentence, there is a "the"
    too many:

        If a service provider cannot be loaded for any of *the* *the* reasons...

  * in the "load" (method-detail for: "public static <S> ServiceLoader<S> load​(Class<S> service,
    ClassLoader loader)" ) description, section "Step 1", paragraph starting with "Ordering:", last
    sentence, a "the" is missing "... in same class loader ...", should read: "... in *the* same
    class loader..."

  * Documentation of "Parameters:" in all of the "load" and "loadInstalled" method-details reads:
    "service - The interface or abstract class representing the service", which may wrongly imply
    that  a concrete class may not be supplied; for completeness of the documentation it should
    document that it may be a concrete class as well or just talk about something like: "Class
    representing the service, usually an interface class" to encourage usage of interface classes

  * in the "findFirst" (method-detail) description, second paragraph, second (last) sentence may
    have an "are" too many: "If there are no service providers *are* located then it uses a default
    implementation."

The text explains ServiceLoader very clearly!

---rony

On 20.06.2017 12:20, Alan Bateman wrote:

> We have two javadoc/spec updates that I'd like to get into the JDK 9 Initial Release Candidate
> that is scheduled for this week.
>
> The spec updates are for two issues:
>
> 1. ServiceLoader: The API spec has been updated significantly to support modules but it needs
> another round of update to do clean-up to get it more readable and consistent, and also to align
> it with the JLS.  Most of reorganization and re-wording has been proposed by Alex. Joe Darcy has
> also proposed a few adjustments.
>
> 2. Upgradable modules aren't specified anywhere. Java SE will designate a number of standard
> modules as upgradeable but we don't have anywhere in the docs to link to that or describe how the
> upgraded versions are used in preference to the modules built into the environment.
>
> The webrev with the proposed (docs only, no implementation) changes is here:
>   http://cr.openjdk.java.net/~alanb/8182482/webrev/index.html
>
> The ServiceLoader diffs are hard to read. It might be easier to read the generated javadoc:
> http://cr.openjdk.java.net/~alanb/8182482/docs/java/util/ServiceLoader.html
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Hamlin Li


On 2017/6/21 3:51, Rony G. Flatscher wrote:

> Maybe a few little things:
>
>    * in the Errors section, reason # 4 states:
>
>          The service provider class file has more than one public static no-args method named
>          "provider".
>
>      There could be no more than one public static no-args method named "provider" in a class file,
>      so this error reason should not be possible?
>
Hi Rony,

It's possible to have 2 public static no-args method "provider" in a
service provider *class file*, JVM spec allows it, as method return type
is also part of method descriptor.

And I still have questions:
Why need to have this restriction in java API doc? Will there be
corresponding update in JVM spec? When will this restriction be
verified, at linking time?

Thank you
-Hamlin
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alan Bateman
On 21/06/2017 07:10, Hamlin Li wrote:
> :
>
> It's possible to have 2 public static no-args method "provider" in a
> service provider *class file*, JVM spec allows it,
Right, you can't do this in the Java Language but the JVMS allows it.
The most obvious usage of this "feature" is covariant return types where
the java compiler will generate a bridge method with the less specific
return type from the superclass. In the Core Reflection APIs then
getMethods and getMethod need to deal with this so that the user of API
only sees the overriding method.


>
> And I still have questions:
> Why need to have this restriction in java API doc?
For completeness only as it's one of the reasons why SCE is thrown. Note
that it's also not "new" in this update, instead the wording for many of
the error cases has been refreshed.



> Will there be corresponding update in JVM spec? When will this
> restriction be verified, at linking time?
No impact.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Hamlin Li
Hi Alan,

Thank you for explanation, it's much clear for me.


Besides of it, I have some minor comments about the doc:


In section "Timing of provider discovery", it says as below:

Each invocation of the|iterator|method returns an|Iterator|that first
yields all of the elements cached from previous iteration, in
**instantiation order**, and then lazily locates and instantiates any
remaining providers, adding each one to the cache in turn.

But in API doc for iterator(), it says:

Caching: The iterator returned by this method first yields all of the
elements of the provider cache, in **the order that they were loaded**.


In API doc for findFirst(), it says:

Load the first available service provider of this loader's service.

By comparing to API doc of iterator() and stream(), should above line be
"Load *and *instantiate** the first available service provider of this
loader's service."


Thank you

-Hamlin


On 2017/6/21 15:51, Alan Bateman wrote:

> On 21/06/2017 07:10, Hamlin Li wrote:
>> :
>>
>> It's possible to have 2 public static no-args method "provider" in a
>> service provider *class file*, JVM spec allows it,
> Right, you can't do this in the Java Language but the JVMS allows it.
> The most obvious usage of this "feature" is covariant return types
> where the java compiler will generate a bridge method with the less
> specific return type from the superclass. In the Core Reflection APIs
> then getMethods and getMethod need to deal with this so that the user
> of API only sees the overriding method.
>
>
>>
>> And I still have questions:
>> Why need to have this restriction in java API doc?
> For completeness only as it's one of the reasons why SCE is thrown.
> Note that it's also not "new" in this update, instead the wording for
> many of the error cases has been refreshed.
>
>
>
>> Will there be corresponding update in JVM spec? When will this
>> restriction be verified, at linking time?
> No impact.
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alan Bateman


On 21/06/2017 09:27, Hamlin Li wrote:

> :
>
>
> Besides of it, I have some minor comments about the doc:
>
>
> In section "Timing of provider discovery", it says as below:
>
> Each invocation of the|iterator|method returns an|Iterator|that first
> yields all of the elements cached from previous iteration, in
> **instantiation order**, and then lazily locates and instantiates any
> remaining providers, adding each one to the cache in turn.
>
> But in API doc for iterator(), it says:
>
> Caching: The iterator returned by this method first yields all of the
> elements of the provider cache, in **the order that they were loaded**
>
There is an inconsistency here, it would be clearer to say that it's the
order that they were loaded and instantiated. This wording has been
there since Java SE 6 but I agree it all needs to be re-examined. So at
some point there will be another big update to the ServiceLoader javadoc
but more likely to be for Java SE 10 given that JDK 9 is at GAC this week.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alan Bateman
In reply to this post by Rony G. Flatscher
On 20/06/2017 20:51, Rony G. Flatscher wrote:
> Maybe a few little things:
>
>    :
>
>    * in the "stream" (method-detail) description, second paragraph, second sentence, there is a "the"
>      too many:
>
>          If a service provider cannot be loaded for any of *the* *the* reasons...
Thanks.

>
>    * in the "load" (method-detail for: "public static <S> ServiceLoader<S> load​(Class<S> service,
>      ClassLoader loader)" ) description, section "Step 1", paragraph starting with "Ordering:", last
>      sentence, a "the" is missing "... in same class loader ...", should read: "... in *the* same
>      class loader..."
Thanks.

>
>    * Documentation of "Parameters:" in all of the "load" and "loadInstalled" method-details reads:
>      "service - The interface or abstract class representing the service", which may wrongly imply
>      that  a concrete class may not be supplied; for completeness of the documentation it should
>      document that it may be a concrete class as well or just talk about something like: "Class
>      representing the service, usually an interface class" to encourage usage of interface classes
This hasn't changed in this update (you'll see the same in Java SE 8)
but I agree it hints that a concrete class is rejected (which it isn't).


>
>    * in the "findFirst" (method-detail) description, second paragraph, second (last) sentence may
>      have an "are" too many: "If there are no service providers *are* located then it uses a default
>      implementation."
>
I noticed this too before pushing the changes so it has been fixed in
jdk9/dev.

-Alan.
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Remi Forax
In reply to this post by Alex Buckley
----- Mail original -----
> De: "Alex Buckley" <[hidden email]>
> À: "jigsaw-dev" <[hidden email]>
> Envoyé: Mardi 20 Juin 2017 20:09:14
> Objet: Re: 8182482: Module System spec updates

> Hi Remi,
>
> On 6/20/2017 6:29 AM, [hidden email] wrote:
>> ok, let's focus on abstract class defining a service.
>
> I would be happy for the "Designing services" section to give more
> advice about the tradeoffs between an interface and an abstract class.
> Two sentences, written in a style that leads a junior developer but does
> not judge them if they don't follow the advice. Can you write it? :-

sure let's try:
if you are wondering when to use an interface and when to use an abstract class, you can use this advice: most of the time you should favor interfaces, unless you want to execute your own code as part of any service implementations like by example when you want to add a security check to validate something about the implementation.

I'm sure you will be able to transform it in proper English.

>
> -----
> A service is a single type, usually an interface or abstract class.
> ***REMI'S TEXT HERE*** A concrete class can be used, but this is not
> recommended. The type may have any accessibility.
>
> The methods of a service are highly ...
> -----
>
> Alex

Rémi
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Hamlin Li
In reply to this post by Alan Bateman


On 2017/6/21 19:43, Alan Bateman wrote:

>
>
> On 21/06/2017 09:27, Hamlin Li wrote:
>> :
>>
>>
>> Besides of it, I have some minor comments about the doc:
>>
>>
>> In section "Timing of provider discovery", it says as below:
>>
>> Each invocation of the|iterator|method returns an|Iterator|that first
>> yields all of the elements cached from previous iteration, in
>> **instantiation order**, and then lazily locates and instantiates any
>> remaining providers, adding each one to the cache in turn.
>>
>> But in API doc for iterator(), it says:
>>
>> Caching: The iterator returned by this method first yields all of the
>> elements of the provider cache, in **the order that they were loaded**
>>
> There is an inconsistency here, it would be clearer to say that it's
> the order that they were loaded and instantiated. This wording has
> been there since Java SE 6 but I agree it all needs to be re-examined.
> So at some point there will be another big update to the ServiceLoader
> javadoc but more likely to be for Java SE 10 given that JDK 9 is at
> GAC this week.
Hi Alan,

Got it, Thank you for planing to update it, especially for "load" and
"instantiate", in several places they are used in a mixed way.

Thank you
-Hamlin
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Alex Buckley
On 6/21/2017 5:34 PM, Hamlin Li wrote:
> Got it, Thank you for planing to update it, especially for "load" and
> "instantiate", in several places they are used in a mixed way.

Yes, it is critical to be clear about:

- when a service provider is discovered (search a family of loaders or
layers, depending on how the service loader was created), versus

- when a service provider is loaded (not by the load* methods! and maybe
the provider is found in the cache...), versus

- when a service provider is instantiated (streaming lets you defer
that, iteration doesn't).

Lots of puzzle pieces that need to snap together.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: 8182482: Module System spec updates

Hamlin Li
Hi Alex,

Yes, that's what I mean.

Thank you

-Hamlin


On 2017/6/22 8:55, Alex Buckley wrote:

> On 6/21/2017 5:34 PM, Hamlin Li wrote:
>> Got it, Thank you for planing to update it, especially for "load" and
>> "instantiate", in several places they are used in a mixed way.
>
> Yes, it is critical to be clear about:
>
> - when a service provider is discovered (search a family of loaders or
> layers, depending on how the service loader was created), versus
>
> - when a service provider is loaded (not by the load* methods! and
> maybe the provider is found in the cache...), versus
>
> - when a service provider is instantiated (streaming lets you defer
> that, iteration doesn't).
>
> Lots of puzzle pieces that need to snap together.
>
> Alex