Request Review: JDK-6479237 (cl) Add support for classloader names

[Daniel Fuchs]

Hi Mandy,

On 01/11/16 23:42, Mandy Chung wrote:
> Hi Daniel,
>
> Here is the updated webrev incorporating your feedback:
>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.03

ClassLoader.java:

  345         if (name != null && name.isEmpty()) {
  346             throw new IllegalArgumentException("name must be 
non-empty or null");
  347         }


I'd suggest passing 'name' to 'checkCreateClassLoader()' and do
this check in checkCreateClassLoader instead - in order to do
the checks before 'this' is created.

[...]

>> Also should there be some asserts somewhere verifying that moduleVersion
>> is null or empty when moduleName is null or empty? At the moment the
>> constructor will blindly accept a version for an unnamed module,
>> and I am assuming this is wrong - am I right?
>>
>
> Good point.  The constructor should throw IAE if module name/version and class loader name is an empty string.

That's not exactly what I had in mind. I don't have
any particular feeling for whether "" should be allowed
or not. Maybe it would be simpler to just allow "" to mean the
same thing as 'null'. What I meant was:
Can moduleVersion be non null (and non empty) if moduleName is null
(or empty)? In other words can an unnamed module have a version?

Also if you add some validation to the constructor then
you will need to add a readObject to duplicate the validation
checks. And if you disallow "" then checking for isEmpty() in
toString() is not needed. But I have the feeling that simply
allowing "" and null to mean the same thing would be more
robust. It's just the question of having a moduleVersion
for an unnamed module that bothers me.

>> toLoaderModuleClassName will call ClassLoader.getName().
>> If a class loader overrides getName() then that might be a different
>> name than what StackTraceElement.getClassLoaderName() returns.
>>
>> Is that an issue?
>>
>
> I added a test to verify StackTraceElement that uses the ClassLoader's name field.  I also added @apiNote in ClassLoader::getName.

Ok - the @apiNote should cover that.

>> Also I wonder whether you should be considering including a fix
>> for https://bugs.openjdk.java.net/browse/JDK-8167099 as part
>> of this change (though arguably the bug has been there since
>> new fields were added to StackTraceElement in 9).
>>
>
> It is related but it’s better to separate it from this issue.  Do you have cycle to help write a test case?  I can help fix JDK-8167099 later.

Right - no problem with that.
I haven't given any thought to a test case for JDK-8167099.
I believe it will prevent to connect a JDK 9 jvisualvm to a
JDK 8 process if not fixed though. Maybe serializing a ThreadInfo
composite data containing some MonitorInfo in JDK 8, and
then deserializing that in JDK 9 and trying to convert it
back to ThreadInfo in 9 would show the issue.

best regards,

-- daniel

>
> Mandy
>