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

[Yumin Qi]

I have to make a correction: DestroyJavaVM thread will not load 
TestClass2 so no CircularityError happened with it. When it is created 
and run, the loading of TestClass2 already finished.

On 10/14/2014 11: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.
> 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 *
>