Re: Change to net.jini API

[Peter Jones <pe...@sun.com>]

On Sat, May 31, 2008 at 05:27:07PM +0200, Mark Brouwer wrote:
> Peter Jones wrote:
>> One preliminary question: could you explain the thinking that led
>> you to making the codebase parameter to the new protected method be
>> a String instead of a URL[] (which will already have been computed
>> at the corresponding points in the implementation, as effectively
>> required by the spec)?  I'm not asserting that this is the wrong
>> choice, just wondering what your motivation was, to help me think
>> about it.  Thanks.
>
> I think the best thing to do is to read these thread from almost a
> year ago as a refresher
> http://mail-archives.apache.org/mod_mbox/incubator-river-dev/200706.mbox/%3c4673FEF7.3010703@cheiron.org%3e
> .

Thanks for the pointer/reminder.

> I hope this makes clear why I came up with the API as it is [1]. I
> think I fail to understand your remark "as effectively required by
> the spec", so could you give me an extra hint here as to what you
> are referring.

I was referring to the "throws MalformedURLException" specified for
PCP's loadClass, loadProxyClass, and getClassLoader methods, which I
interpret to mean that the codebase argument must be converted to URLs
in any case, early on.

> [1] from that thread I can understand people fail to see why the
> signature doesn't have both the codebase string and URL array but I
> decided in that year that I didn't like that.

My hesitation on what the signature of this new protected method
should be is partly related to the fact that PCP's implementation of
the proposed version makes use of facilities not exposed to
subclasses:

- the pathToURLs cache, both for the (String) codebase argument as
well as for the loader's annotation

- a straightforward way to get the annotation string for the loader.
The public getClassAnnotation(Class) method is not straightforward
because it requires a Class defined by the loader, and it checks
permissions for URLClassLoaders.

So if a subclass wanted to, e.g., customize the URL path matching
function but not the loader->annotation mapping, which I think is
closer to the original spirit of RIVER-147 (Bob?), that wouldn't be
straightforward to do with this hook.  I gather that you do want to
customize the loader->annotation mapping, which is why the loader
parameter is essential for your purpose.

This makes me wonder, though-- you must be overriding
getClassAnnotation(Class) too, right?  Would you be happier with a way
to override the getLoaderAnnotation method, which would cover both the
getClassAnnotation(Class) case and the boomerang matching case?  And
with such a method exposed to subclasses in addition to the proposed
one, a different subclass could more easily override just the matching
function.  (Of course, there is the fear that subclass contract design
in general is strewn with land mines, exponentially related to the
number of API elements involved in the contract...)

Following are the rest of my comments based on the currently proposed
API changes:

There will need to be some additional spec updates-- in particular, I
think that all references to a loader's "annotation URL path" will
need to be replaced:

- Conditions based on the equality of a URL path to a loader's
annotation URL path will need to be replaced with conditions based on
the result of invoking the new protected method.

- The remaining case is the definition of "permission to access the
codebase loader".  With this change, it seems appropriate to replace
"codebase loader's annotation URL path" with "codebase URL path"-- no
longer the same thing in applicable cases-- because the latter are the
URLs whose permission will actually be checked.

With those replacements, it seems that the definition of a loader's
"annotation URL path" can be removed, although perhaps some of its
words should go into the specification of PCP's implementation of the
protected method.

The proposed spec for the new method mentions the term "class
boomerang", but that's not a term defined or used elsewhere in the
spec, and I'm inclined to keep it that way.  :-)

Since the 2.1 version, I think that all of PCP's spec words are in my
"voice", so if you like, for consistency there, I would be happy to
supply final words for these spec updates, once the signature and
semantics are settled.

Regarding the proposed name of the new protected method:

+    protected boolean isClassDefinedInClassLoader(String codebase,
+                                                 ClassLoader loader) {

A long time ago, a certain JVM spec author drilled into my head that a
class is not defined "in" a class loader, it is defined "by" a class
loader, so I don't like the use of "in" here.  Moreover, I agree with
a previous comment that it seems odd for the name to mention a class
when the parameters do not identify a particular class.  I suggest the
name "isCodebaseForClassLoader".  Thoughts?

Regarding the proposed changes to the checkLoader method: it seems odd
to me for the (optionally-enabled) assertion to invoke this
subclass-overridable method-- especially since the "functional"
requirements of the method are not entirely clear.  I would just
remove this assertion from checkLoader, and then its parameter list
can remain as it was.

@@ -481,7 +481,6 @@
        }
 
        URL[] codebaseURLs = pathToURLs(codebase);      // may be null
-                                       // throws MalformedURLException

It's a trivial matter, but I have to ask: why these comment removals
(in three places)?  They were there to clarify for the reader/author
where the MalformedURLException requirement for these methods is
implemented.

Remaining code changes look OK to me.

-- Peter