RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

David,

   I will take another detail trace to see where the exception begins in 
main thread, it should not thrown in main thread. I only saw it is 
thrown in TestThread, not main, not DestroyJavaVM. If that happens, 
maybe something wrong in vm.
   The output in all 'failed' case (many failed not cause exception 
output, not caught), the main thread got the exception. That is not right.

Thanks
Yumin

On 10/14/2014 5:28 PM, David Holmes wrote:
> Hi Yumin,
>
> On 15/10/2014 4:40 AM, Yumin Qi wrote:
>> David,  Thanks for the comment. See embedded.
>>
>>
>> On 10/13/2014 7:30 PM, David Holmes wrote:
>>> Hi Yumin,
>>>
>>> jdk9-dev is not the best place for code review requests.
>>> serviceability-dev would be better for this test.
>>>
>>> On 14/10/2014 8:58 AM, Yumin Qi wrote:
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8038468
>>>> webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/
>>>>
>>>> the bug marked as confidential so post the webrev internally.
>>>
>>> Not any more :)
>>>
>> Thanks. I changed to non security related bug. Usually when test failed,
>> a confidential bug is filed. I would like to create bug open if the test
>> is in open part.
>>>> Problem: The test case tries to load a class from the same jar via 
>>>> agent
>>>> in the middle of loading another class from the jar via same class
>>>> loader in same thread. The call happens in transform which is a rare
>>>> case --- in middle of loading class, loading another class. The result
>>>> is a CircularityError. When first class is in loading, in vm we put
>>>> JarLoader$2 on place holder table, then we start the defineClass, 
>>>> which
>>>> calls transform, begins loading the second class so go along the same
>>>> routine for loading JarLoader$2 first, found it already in placeholder
>>>> table. A CircularityError is thrown.
>>>> Fix: The test case should not call loading class with same class 
>>>> loader
>>>> in same thread from same jar in 'transform' method. I modify it 
>>>> loading
>>>> with system class loader and we expect see ClassNotFoundException.
>>>> Detail see bug comments.
>>>
>>> It is not clear to me that the test is incorrect. It is also unclear
>>> why such an old test is now failing - we must have changed something.
>>> And it's unclear whether what the test does with your change is
>>> actually testing what the test wanted to test.
>>>
>>> It seems to me that the actual problem in the test is the reference to
>>> the "main" thread ie:
>>>
>>>  if (!tName.equals("main"))
>>>
>>> The test knows not to do the loading in the main thread, but has
>>> overlooked the fact that the main thread, upon the end of main()
>>> becomes the DestroyJavaVM thread - and it is that thread which
>>> encounters the ClassCircularityError:
>>>
>>> Starting test with 1000 iterations
>>> Thread 'DestroyJavaVM' has called transform()
>>>
>>> So perhaps the right fix is to expand the above to:
>>>
>>>  if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))
>>>
>>> ? I admit I'm having trouble seeing the full picture in this test.
>>>
>> It is not DestroyJavaVM thread cause CircularityError. It is TestThread
>> cause CircularityError.
>
> Not according to the bug report:
>
> Starting test with 1000 iterationsThread 'DestroyJavaVM' has called 
> transform()
> Thread 'DestroyJavaVM' has called transform()
> result=1
> ----------System.err:(14/920)----------
> Exception in thread "main" java.lang.ClassCircularityError: 
> sun/misc/URLClassPath$JarLoader$2
>
> This shows that "main" got the CCE. Which in itself is confusing given 
> we also report "Thread 'DestroyJavaVM' has called transform()" and 
> they are in fact the same thread!
>
> David
> -----
>
>
>> In TestThread (DestroyJavaVM may cause same I think, but not seen in
>> debug):
>>
>>      forName("TestClass2", true, classLoader);  <---- the loader is
>> customer loader which is obtained from agent code.
>>           -->...... transform(...)
>>               -->defineClass(...)
>>                     -->...... call into vm, we need to load JarLoader$2
>> since JarLoader$1 used
>>                         ->resolve_instance_class_or_null
>>                                 // here we create PlaceTableEntry for
>> JarLoader$2, put into place holder table
>>                             -->......
>>                                 --->forName("TestClass3", true,
>> classLoader);
>>                                     -->... transform(...)
>>                                         -->defineClass(...)
>>                                             -->...... call into vm
>> again. Now JarLoader$2 is not loaded, but it is in placeholder table, so
>> throw_circularity_error set and throw.
>>                        .......
>>       With custom loader, agent's transform will be called, then it
>> loads TestClass3, repeat the same steps as loading TestClass2. The
>> problem is JarLoader$2 has not been loaded yet but in place holder table
>> (this is for checking CircularityError), then begins loading TestClass3,
>> this is a recursive and embedded case.  The non-failed case also saw
>> CircularityError thrown, but somehow the test case did not fail.  Design
>> like this will cause call transform in transform which is the reason
>> CircularityError thrown.
>>
>>     I have no idea about the original desin of the test case, but think
>> it should do this.
>>
>>>
>>> Looking at your change, don't leave commented out lines in the code:
>>>  115                         // ClassLoader loader =
>>> ParallelTransformerLoaderAgent.getClassLoader();
>>>  118 //Class.forName("TestClass" +
>>> index, true, loader);
>>>
>> Will remove
>>
>> Thanks
>> Yumin
>>> Thanks,
>>> David
>>>
>>>> Thanks
>>>> Yumin *
>>