#LayerPrimitives should be re-opened

[Thomas Watson]

Comments inline.

Tom

> From: "David M. Lloyd" <david.lloyd at redhat.com>
> I am requesting that the #LayerPrimitives issue be re-opened.
> 
> This issue requested the ability for Layer.Controller to perform the 
> following operations, which Modules can already do for themselves (and, 
> by extension, Layers can do as well using bytecode injection):
> 
> • addExports(Module source, String packageName, Module target) - 
> equivalent to Module#addExports(String packageName, Module target), 
> "source" must be in the controller's corresponding layer
> • addUses(Module source, Class<?> service) - equivalent to 
> Module#addUses(Class<?> service), "source" must be in the controller's 
> corresponding layer
> 
> These two methods should be added because requiring bytecode injection 
> to perform these functions is unreasonably onerous, which alone should 
> be enough to justify re-opening the issue.

This seems reasonable to me given that a module can perform these 
operations themselves it makes sense to allow the controller to do it 
also.

I think the addUses would be more useful if we had a way to know when a 
module is trying to load a new ServiceLoader class.  This way we could 
dynamically add the required uses just in time before the loader did its 
check to see if the module can use the service.

> 
> The following self-explanatory operations do not have corresponding 
> methods on Module, and normally can only be done by editing the 
> descriptor or using the Instrumentation#redefineModule(...) API to do 
so:
> 
> • addOpensToAll(Module source, String packageName)
> • addExportsToAll(Module source, String packageName)
> 
> It is not always possible for a container to know what packages need to 
> be exported or opened to all when the module is built, especially in a 
> dynamic deployment environment like ours.  Note that 
> Controller#addOpens(Module source, String packageName, Module target) is 

> already an existent method on Controller thus is already deemed "safe".
> 
> The following operations neither have corresponding methods on Module, 
> nor can be done by editing the descriptor AFAIK:
> 
> • addOpensToAllUnnamed(Module source, String packageName)
> • addExportsToAllUnnamed(Module source, String packageName)
> 
> The latter is arguably not necessary since unnamed modules are usually 
> not going to be bound by JPMS linkage, so from what I understand, the 
> exports usually won't apply in this case anyway. 

This is not my understanding.  Unnamed modules can read any other module, 
but they still can only load classes from packages that are unqualified 
exports already.  But I do question if the addExportsToAll and 
addOpensToAll include unnamed modules or not.  If they do then it seems 
this extra method to only do it for unnamed modules is not needed.
 
> It seems to me that 
> the former method might be of use in some cases; having unnamed module 
> code reflecting on module code does not seem very outlandish to me, 
> especially in a container environment.

Perhaps, but I wonder why the controlling layer did not just make the 
modules within the layer open modules so it did not have to do this 
dynamically later.

> 
> This leaves addPackage(Module source, String packageName).  This method 
> is not available on Module and can only otherwise be done by editing the 

> descriptor or using Instrumentation#redefineModule(...).  However, if 
> your loader does not yet know the complete set of packages (i.e. it's 
> lazy), or the set of packages might be supplemented after the module is 
> initially created, you either need this method, or you have to redesign 
> your container's linkage strategy from the ground up.  We definitely 
> need this method.  One OSGi implementer has indicated that they can live 

> without it (by not implementing some optional pieces, and mandating 
> strict behavior where other containers had flexibility previously) but 
> I've also reached out to a few others to see if I can get more data 
> about this.  I know that JBoss OSGi does presently rely on this sort of 
> functionality already; I am not sure if it is limited to fragment 
> support or if there is more to it.  I have a query out to the maintainer 

> of that project (Arcadiy Ivanov) to get further information, but I 
> understand he is traveling at the moment.

This method would help for the dynamic fragment attachment case in OSGi. 
Also required for that would be the ability to addExportToAll.  Having 
addPackage without addExportToAll would be an incomplete solution for the 
dynamic fragment attachment case in OSGi..

> 
> However, the potential of the OSGi requirement is not a necessary 
> condition for this method to be justified.  We believe that it is also 
> very useful when delivering a Java EE container; we have relied on this 
> functionality for many years.  There are not many Java EE certified 
> vendors in the world, so I would consider this alone to be just as 
> substantial a justification for the feature.  Logically it is possible 
> to deliver a Java EE container without it, just as it is possible to 
> deliver an OSGi container without it, but this means the end of certain 
> features which a massive number of users have relied upon.
> 
> This excludes other possible uses, like: dynamic code discovery (via 
> network for example), IDE code hot-deployment features which would 
> require Instrumentation otherwise, etc.
> 
> The patch itself (still viewable here [1]) is very small, mostly 
> consisting of JavaDoc, and only utilizes existing functions in Jigsaw. 
> It does not introduce any new implementation behavior, and is only 
> available to Layer implementers, therefore I would consider the risk to 
> be very small by any concrete or objective measure.
> 
> These primitives also have the advantage of enabling an acceptable 
> solution to the following issues (at minimum):
> 
> • #NonHierarchicalLayers
> • #CyclicDependences (at least, within containers)
> • #MutableConfigurations
> 
> And can be used to provide container-based solutions to several others 
> as well.  To me this looks like a very big reward for low risk.
> 
> My patch also proposed a trivial one-line fix to remove a double null 
> check from the existing Controller#addOpens(Module, String, Module) 
> method.  This may have been fixed upstream already, but because a storm 
> has disabled my network connection, I cannot verify this easily at 
present.
> 
> If the problem with the issue is the total set of methods, I'm OK with 
> dividing the issue up and arguing the merits of each.
> 
> I think this issue should be re-opened, and I think that doing so, with 
> a view towards satisfactory resolution, is in the best interests of the 
> community as well as the majority of the members of this expert group.
> 
> [1] https://gist.github.com/dmlloyd/e3c114f0ac7595a93ecb63b1dbed5e00
> 
> -- 
> - DML
>