Hey Stanley,
More comments inline.
// Bryan
Stanley M. Ho wrote:
Hi Bryan,
Thanks for the inputs. See my comments inline.
Bryan Atsatt wrote:
Stanley,
Here are my comments on both the spec and the apis.
// Bryan
SPEC
====
1.5
You are assuming that module-name==superpackage-name, right? Otherwise,
if we add import statements to 294, then these would have to specify
module names, thus creating a circular dependency.
Right. I will clarify it in the next revision of the spec.
2.1
"is properly understood as a mathematical abstraction"
This phrase seems unnecessary, and comes across as a bit highbrow to
me.
"A module archive is inherently sealed, and it does not reference any
resource externally"
The term "sealed" here could be confused with the manifest attribute.
Perhaps a better term is "self-contained". And the second phrase is a
bit confusing also, since imports do reference external "resources".
Perhaps if you said "may not directly reference external classes or
resources"?
I will look for better words to describe it.
2.2
Should state that module name must == superpackage name.
2.4
The members definition should be a list of *packages*, not classes.
That
is all the 294 proposal supports (I actually can't see it at the moment
since my HD died, but this is my recollection!). I think it is
reasonable to assume it will stay this way.
The member declared in the source file is indeed a list of packages.
That said, based on my understanding of 294, the compiler would expand
the member packages to a list of classes in the artifact. I will clarify
it in the spec.
Version may be optional in a superpackage, but it is mandatory for a
module, right? This should be clarified.
"mandatory" would mean something that we can check and enforce. Since
version is declared using annotation in a superpackage, I don't think we
can enforce it at build time. That said, we can still enforce it at
deployment time if this is what you meant.
Yes, the 277 spec should describe it as mandatory, and we should enforce
it at runtime. If it becomes possible to enforce it a compile time, we
would do so.
The example members list should be package names only.
2.7.3
I like this addition.
I'm glad to know.
2.7.4
There may well be cases in which a module wants to re-export a
subset of
imported classes/resources. We should consider supporting this case.
Could you describe the actual use cases for this?
Package a.b contains and exports classes C and D.
Package x.y imports a.b, but only wants to re-export C.
4.1
Since not everyone uses the Sun terms (FCS, GA), we might want to also
define qualifier here as "beta", or "pre-release", or
"release-candidate". And a "release" version must not have a qualifier.
I will clarify it.
5.2.2
We should also make it clear that resources will also often be
contained
in "legacy" jars, for which no ClassesDirectoryPath is required.
There is related to an open issue on how we want to support "legacy"
jars in the implementation. I will open up a new thread for the
discussion on this topic.
5.6
Might be a good idea to explicitly state that the Class-Path attribute
is ignored.
Right.
#3 This would appear to rule out OSGi's support of split packages. This
spec should enable each ModuleSystem to control this behavior.
The JAM file is the distribution format for the module system defined by
277, and I don't think the treatment of JAR manifest in this section
applies to OSGi.
Sure, for the manifest of a .jam file. But the statement:
"All packages defined in a module definition are inherently sealed, and
this entry is ignored by the module system."
is pretty broad, and would seem to indicate that it applies to the
definitions produced by any module system.
6.1
Third paragraph: Remove the leading "Besides, "
6.2.2
Repository shutdown must define the semantics for any module instances
from that repository.
I still strongly believe we need a module shutdown method that
optionally disables the loader.
And this should also be used during repository shutdown. See my
comments
below for section E.5.
This is on my radar, and is briefly mentioned in E.4 and E.5. We will
need to discuss what kind of lifecycle support we want to provide, and I
will open a new thread for the discussion soon.
6.2.3/6.3
For EE and other similar systems, it may be useful to have different
VisibilityPolicy instances per Repository. We may want to have a
getter/setter here, with the default implementation of get returning
the
default policy.
Could you describe the use cases you had in mind for this?
It creates a nice way to create wrapper repository instances that
provide a customized view...
EE systems are required to isolate applications from each other. And
each may have very different external dependencies. If each repository
instance can have its own VisibilityPolicy, then a wrapper Repository
can be constructed for each application, using a different policy.
Same goes for ImportOverridePolicy.
6.2.6
Local repository instances will "know" when any of these events occurs
(via their API). Remote ones will not, so reload() is a reasonable
method.
Should reload be *required* before any repository returns new/modified
instances?
My expectation is that most repository implementations would want to
optimize their performance by caching most things in memory and not poll
the underlying storages unless they have to, so I think "reload" should
be required. That said, if you have use cases that show otherwise, I
would be interested to know about them.
I'm just considering the simple case of deploying a new module to a
LocalRepository. I think it will be a bit surprising if some special act
is required to make it available. For example, this would fail:
localRepos.install(aModuleNamedSue);
ModuleDefinition sue = localRepos.find("sue");
And it is pretty awkward to insert a call to reload() here. Given that
the primary client of Repository is ModuleSystem, I guess the
preparation logic can call reload() and re-try IFF a suitable definition
cannot be found.
6.5.1
It isn't clear from this that each repository instance sharing an
interchange dir must have a unique "expansion" directory.
While some variant subclass could be created that uses appropriate
locking to share the expansion dir, the provided LocalRepository does
not.
I think this will be up to each repository implementation to decide. No
matter what the implementation strategy is, it should not affect the
repository's semantic.
Yes, but LocalRepository is a specific implementation, and one that
requires an expansion directory for each instance, right? We should make
that clear.
6.5.3
#5 Should be "shut down" not "shutted down".
7.1.1/7.1.2
Should say that a system property may be used to control the type
returned by ModuleSystem.getDefault().
This method would be used to return the default module system which has
ties with the underlying JRE implementation. As we discussed, this is
not replaceable, so I don't think providing a system property for
override makes sense. ;)
Thinking about this a bit more, the idea of a "default" module system is
a bit odd. Certainly there will be one module system used by the JRE
itself, and this is what you were thinking about here. And clearly
LocalRepository and URLRepository are hard-wired to this same module
system, so it makes some sense to be able to say in the javadoc for
these classes that they use the "default" module (which needs to be
added, btw). Do you have any other uses in mind? Perhaps we should be
more explicit:
public static ModuleSystem getJREModuleSystem();
Regardless, what I was really thinking about before is ModuleSystem
initialization.
ModuleDefinition has a getModuleSystem() method, but how is it
implemented? Our model so far appears to assume that ModuleSystem
instances will be shared, which is certainly reasonable, but... how? One
simple model is that it is module system dependent, e.g. each module
system implementation provides a singleton or equivalent. But this just
pushes the problem up a layer, since the runtime type of
ModuleDefinition will vary based on module system.
I see that you added a ctor arg/getModuleSystem() method to Repository
as well. In this model, the type problem is pushed up one more layer. It
is the Repository implementation that must know the ModuleDefinition
type, and therefore the ModuleSystem type.
But we need to specify exactly how the initial repositories are
configured/initialized, using the correct types, so that the JRE can
instantiate them. (I took a cut at this in the class loading doc, if you
recall.) This should be added to Appendix E--even if some of the details
are JVM vendor specific, the requirements must at least be defined.
But... didn't we decide long ago that a repository should not be
restricted to a single module system? With a multi-repository search
model, this is less of an issue, but why impose this limit? Composite
implementations might have a hard time implementing getModuleSystem().
Perhaps we should consider a model in which we:
1. Require ModuleSystem implementations to have a globally unique name.
(There aren't likely to be hundreds of them, so this shouldn't be much
of a hardship!)
2. Have a persistent configuration mechanism for ModuleSystems, like the
one for the initial Repository instances. In addition to the runtime
types for ModuleSystem subclasses, the instances themselves will likely
need configuration, just like Repositories (logging, security,
import/visibility policies, etc.)
3. During startup, the JRE initializes the registered ModuleSystems,
then the registered Repositories.
4. Add to ModuleSystem:
public abstract String getName();
public static ModuleSystem getModuleSystem(String name);
public static List<ModuleSystem> getModuleSystems();
Now repository implementations are not involved in *instantiating*
ModuleSystems; they look them up by name.
(and having a centralized lookup mechanism may enable other interesting
behaviors)
I think we need more flexible mechanisms to control this. At the very
least, there should be a way to define it in the local JRE in a
persistent fashion.
But we also may want a way for the launcher to choose the default
module
system based on some delegation model. Perhaps the available systems
can
be specified in a config file, each in a standard 277 module. And the
launcher's search algorithm could iterate across the ModuleSystem
instances and ask each to try loading the main module.
Obviously the specifics need to be worked out, but you get the idea.
This will depend on the implementation logic in the launcher in the JDK,
but the launcher is out of scope in the 277 spec. On the other hand, if
the launcher requires APIs from 277 in order to do its work, then we
will need to consider the requirements.
7.3.1
#4, bullet 2: loadClass() must not mask any thrown Error or
RuntimeException by converting it to a ClassNotFoundException! This
would make debugging the problem much more difficult, and would be a
significant change in class loader behavior.
I will make the logic more clear.
7.3.2.2
The search order for resources MUST be the same as that for classes. I
can support this statement if you like.
How to actually support the notion of exported resources is another open
issue, and I will open a new thread on this soon.
We should also say something about the protocol for resource URLs.
Shouldn't we? Like whether there will be a new protocol for them, or not.
E.5
Again, module shutdown can only be performed within the scope of some
larger construct, such as an EE application/container.
Such an environment can and does have an explicit lifecycle model,
including threading.
Consider what can (and will!) happen after a module uninstall, if that
module is left "alive"...
If the uninstall removes the deployed module from the storage, then
subsequent subsequent attempts to load classes/resources from that
module can fail (depending on what is cached by the underlying
ZipFile/file system/OS implementation).
And it doesn't need to be a direct call to loadClass(). I have often
seen this kind of failure happen with lazy dependency loading.
So, the uninstall either must defer physically removing the deployed
module until all instances are gone, or it must schedule them for
removal on next startup, or whatever... crazy complexity.
Can the storage of a shutdown repository be safely deleted? Not if any
process is still using it.
For all of these reasons, I strongly believe we need a module shutdown
method. And yes, its use must be well documented!
If I understand your requirements properly, I think what you are really
asking is a classloader disable/shutdown method, and such a method would
be called when a module is released (or shutdown) from the module system
in the context of EE servers.
API
---
Since the APIs are still evolving significantly, I would like to defer
most of that discussions until the next revision of the spec and APIs
are available for the EG to review. That said, I will take your comments
into account when the APIs are refactored.
- Stanley
ModuleDefinition
* The getModuleDefinitionContent() method must return a list of
instances, since multiple jars/directories may be present. Might also
make sense to shorten the name to getContents() since the return type
makes it pretty clear what you are getting.
ModuleDefinitionContent
* The semantics of the open() method are fuzzy: who calls it, and when?
Can it be safely called multiple times? What happens when the other
methods are called and open() hasn't occurred?
* Perhaps the open should be automatic on use of any accessor method if
needed?
* There should be an isOpen() method.
* I think the name should be shortened to ModuleContent. It is not
actually part of the definition.
JamsModuleDefinition
* Should be singular: JamModuleDefinition
ImportPolicy
* Should have a static getDefault() method.
Version/VersionRange/VersionContstraint
(See proposal in earlier email)
PlatformBinding
* Why not just call this "Platform"? It is no different than Version,
just another attribute that can be used to filter during resolution.
ModuleArchiveInfo
* Need a way to get this from either a Module or a ModuleDefinition.
Repository
* Implementations of findModuleDefinition() will want to discover if
the
query contains a module name (equality test only!), so that the name
can
be used as a primary key. Either we should add a method to Query to
test
for this, or we should pass a name argument to this method. In the
latter case, a null name must be allowed.
