runtime changes for services

[Mandy Chung]

Alan,

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;
   }

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