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 *
