runtime changes for services

[David Holmes]

On 16/02/2012 9:04 AM, Mandy Chung wrote:
> The new module-info.java files with services look good. This
> changeset looks good as the first installation.
>
> I view "requires service S" as expanded to a set of requires
> clauses on the module view that provides S at install time.
> For example,
> module M {
> requires service S;
> }
>
> If there are 3 different modules PS1, PS2, PS3 providing the
> implementation of service S, it'd be like:
> module M {
> requires PS1;
> requires PS2;
> requires PS3;
> }

Do you mean that PS1, PS2 and PS3 all combine to provide service S? Or 
that they each provide service S?

David
-----

> assuming there is no conflict among these dependencies.
> Synthesizing view dependencies makes sense to me.
>
> The synthesized dependency with no version query (Resolver.java
> L327) leads me to rethink and wonder if the resolver has to take
> services as a constraint.
>
> module M {
> requires MS;
> requires service S;
> }
>
> module MS @ 2.0 {
> // no provides service S
> }
> module MS @ 1.0 {
> provides service S with Impl;
> }
>
> The synthesized dependency is "requires MS" with no version
> query. What would happen in this case? I suspect the resolver
> will fail as it chooses MS at 2.0 but it doesn't provide service S.
> As sthe ensureServicesPresent() method will catch if there is
> any service dependency not satisfied, this might be adequate.
> This seems to be a discussion point for later.
>
> On 2/9/2012 9:06 AM, Alan Bateman wrote:
>> http://cr.openjdk.java.net/~alanb/jigsaw-services/webrev/index.html
> com/sun/classanalyzer/ModuleInfo.java
> L46: you're right that it needs to make a defensive copy
> of services and providers. L45 makes a copy of the requires
> and put it in a TreeSet so that ModuleInfo.getRequires()
> returns an ordered set for writing to module-info.java.
>
> L155-172: would it be better to sort the list before printing
> the "requires services" statements? I believe the ordering of
> "provides service" statements matters and so it should keep
> it the same order as specified in the META-INF/services files.
>
> java/util/ServiceLoader.java
> L357-359: for maintainability, would it be better to replace
> these lines with
> if (!hasNext()) {
> throw new NoSuchElementException();
> }
>
> org/openjdk/jigsaw/Context.java
> L173, 203: I assume you want to fix these methods to return a
> unmodifiable map, right? I don't see the code that expects
> them to be modifiable (maybe I miss something)?
>
> org/openjdk/jigsaw/Linker.java
> L296: extra blank line
>
> org/openjdk/jigsaw/Loader.java
> L286-289: can these lines be replaced with:
> Set<String> remotes = context.serviceSuppliers().get(cn);
>
> L278: I wonder if this should return a map of ModuleClassLoader
> rather than ClassLoader just be explicit.
>
> L291: this causes the module loader for the service
> implementation class be instantiated. I presume we want
> the loader be instantiated lazily when it's loaded. On
> the other hand, the instantiation cost doesn't look like
> too high. It's fine for the initial implementation but
> we should look into this open issue.
>
> org/openjdk/jigsaw/Resolver.java
> L320,322,323, 338: a space below ":"
> There are other places missing the space in the foreach
> statements needed fixing too.
>
> sun/tools/jar/Main.java
> L785: would you suggest to replace Charset.forName("UTF-8")
> with StandardCharsets.UTF_8 (no real difference though)?
> L989: This is not really a question for this change but
> you may have the answer. I have seen some copy of readAllBytes
> in other places. Have we considered adding this support
> to the platform?
>
> new services tests
> Looks good. The copyright start year should be 2012 :)
> I notice that some tests turn on assertion but some don't.
> It might be good to set -esa -ea on all tests I think.
>
> Mandy
>