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

[David Holmes]

Hi Mandy

On 29/03/2018 11:33 AM, mandy chung wrote:
> 
> 
> 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 is not relevant here as we are dealing with classloader names, not 
modules.

>> 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.

We're talking about unnamed classloaders not the unnamed modules.

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

The intent is not to imply that you can tell how/if a loader delegates 
simply by listing its parent. The listing of the parent simply serves to 
tell you what the parent is - no more, no less.

Cheers,
David
-----

> 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