RFR(M): 8199852: Print more information about class loaders in LinkageErrors.

[mandy chung]

On 3/27/18 9:22 AM, David Holmes wrote:
> Hi Lois,
>
> <trimming>
>
> On 27/03/2018 5:58 AM, Lois Foltan wrote:
>> On 3/25/2018 5:48 PM, David Holmes wrote:
>>> Maybe it makes sense to have a set pattern for this descriptive 
>>> text, and use "unnamed" if the loader has no name:
>>>
>>> loader <name>, instanceof <class>, child of loader <name> ...
>>>
>>> not sure how much detail about the parent is needed.
>>
>> I do not support the above pattern for the descriptive text. The 
>> supported pattern has already been documented within the API Note of 
>> StackTraceElement.toString(), 
>> https://docs.oracle.com/javase/9/docs/api/java/lang/StackTraceElement.html. 
>
>
> Sorry Lois but I don't see that the toString() method of 
> StackTraceElement is an authoritive definition of how loaders should 
> be described in other contexts. 

I think Lois suggests to consider the well-defined pattern used in 
printing stack trace when appropriate.  It's designed to be compact and 
provide the adequate information for troubleshooting.

Reading JDK-8199940 and JDK-8199852, a relevant format is the exception 
message in IllegalAccessError (and IllegalAccessException thrown by core 
reflection) that I have added a comment in

class $FROM_CLASS (in $FROM_MODULE) cannot access class $TO_CLASS (in 
$TO_MODULE) because $TO_MODULE does not export $PACKAGE_OF_TO_CLASS to 
$FROM_MODULE

The format of the module is:
    if it's a named module, it will print as "module $MODULE"
    if it's an unnamed module, it will print as "unnamed module 
@0xnnnnnnnn" where 0xnnnnnnnn is the identity hash code of the module.

A module is defined to one class loader.
> That toString definition has to combine module, loader, class and 
> method, information in a compact form. But that is not what we are 
> dealing with here. Had I been involved in that work I would have also 
> argued against completely omitting loader information in the un-named 
> case.

Omitting the builtin class loader has no loss of information for 
troubleshooting. The boot loader, platform loader  and app loader each 
has one unnamed module.   The packages defined to a named module are 
fixed.   It may not look obviously to which class loader/unnamed module 
the class of the stack frame is defined.
>
>> For example, the preference as stated for "unnamed" loaders is:
>>
>> If the class loader is abuilt-in class loader 
>> <https://docs.oracle.com/javase/9/docs/api/java/lang/ClassLoader.html#builtinLoaders>or 
>> is not named then the first element and its following|"/"|are omitted 
>> as shown in "|acme at 2.1/org.acme.Lib.test(Lib.java:80)|"
>>
>> Again, Klass::class_loader_module_name() already implement and adhere 
>> to this pattern.
>
> Yes but we are not dealing with module names here. And the fact it 
> completely ignores un-named loaders makes it inherently unsuitable for 
> the current context.
>
> The issue of whether information about the parent loader is suitable 
> for the error message or should be left for logging, depends on how 
> you expect these messages to help. If the idea is for the exception 
> message to provide enough information for a support engineer to be 
> able to diagnose a problem without asking the submitter to re-run with 
> logging enabled, then you want to gather as much relevant information 
> as possible. But a line has to be drawn somewhere. I don't think 
> listing the loader's parent is excessive in this case, but nor do I 
> see it as essential.
>

I share Lois's concern in including the parent loader in the error message.

The parent loader does not cover all cases.   A well-defined class 
loader follows the parent class loader delegation model.  But there are  
custom class loaders that can delegate to more than one class loaders.  
ClassLoader::getParent does not show the class loader delegation it 
uses.  The class loader defining a named module may delegate to one or 
more class loaders, each is the class loader defining one module 
dependence.  If M requires M1 and M2, M1 and M2 may be defined to 
different class loaders while M's loader::getParent may return a class 
loader that is not M1's and M2's loader.

In any case, improving the error message for LinkageError is good but it 
probably worths taking the time to agree on the format for a named and 
unnamed class loader and its type.  I will give some thought and add the 
JBS issue of any suggestion.

Mandy