[PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently
Eric Wang
yiming.wang at oracle.com
Fri Jun 29 08:31:02 UTC 2012
Hi David,
I have made changes by following your suggestion. Can you please help to
review again? Thanks a lot!
I also change the code of 7123972 which is sent in another mail.
Regards,
Eric
On 2012/6/29 11:04, David Holmes wrote:
> On 29/06/2012 12:10 PM, Eric Wang wrote:
>> Hi Stuart,
>>
>> Thank you, The fix looks simpler than the previous one, it shoud be
>> better to add a Thread.sleep(20) in the loop to avoid multiple calls on
>> System.gc();
>
> Oops I missed the sleep out of my suggestion too.
>
>> Here is a small question if it is possible that after GC, the strong ref
>> remains but somehow weakref is broken，so the loop will terminate and
>> the test will fake pass.
>
> As I said previously this isn't trying to test the GC. Broken weakrefs
> should be detected by weakref tests.
>
> David
> -----
>
>>
>> while(!finalized) {
>> System.gc();
>> Thread.sleep(20);
>> if(ref.get() == null) { //strong ref remains but somehow weakref broken
>> break;
>> }
>> }
>>
>> if (ref.get() != null) { //if weakref broken, test will fake pass
>> throw new Error("TEST FAILED: impl not garbage collected");
>> }
>>
>> Thanks,
>> Eric
>> On 2012/6/29 5:54, David Holmes wrote:
>>> Hi Stuart,
>>>
>>> I don't think finalization or reference queues are needed in this
>>> case. We could simply do:
>>>
>>> while(true) {
>>> System.gc();
>>> if (weakRef.get() == null)
>>> break;
>>> }
>>>
>>> We have to remember that this is not testing the correct operation of
>>> weak references, nor of finalization, but simply that no unintended
>>> strong references remain to the object referenced via the
>>> WeakReference.
>>>
>>> So if a strong ref remains, or somehow weakrefs are broken, then the
>>> loop will not terminate on its own and we rely on the test harness
>>> timing it out.
>>>
>>> David
>>> -----
>>>
>>> On 29/06/2012 7:20 AM, Stuart Marks wrote:
>>>> And here's the webrev for this one:
>>>>
>>>> http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.0/
>>>>
>>>> Also looks good to me. It's pretty similar to the other fix (7123972).
>>>> If there are no further comments I'll push this in the next day or so.
>>>>
>>>> * * *
>>>>
>>>> Some further comments, mainly aimed at the likes of David Holmes. I'm
>>>> mostly "thinking out loud" at the moment.
>>>>
>>>> While I'm glad to see the reliability of these tests improved and
>>>> to see
>>>> them come off the problem list, it would be nice to see something more
>>>> general that could be shared among other tests, and possibly more
>>>> abstract so that things can be tuned to different VM
>>>> characteristics if
>>>> necessary.
>>>>
>>>> Basically what these tests want to do is to make sure that a certain
>>>> object gets garbage collected. I think this is pretty common. There
>>>> are
>>>> several dozen tests in the jdk repo that have the word "leak" in
>>>> them. I
>>>> bet they all use different techniques to detect leaks. :-(
>>>>
>>>> The technique used here is to get a weak reference to the object, and
>>>> then use the object's finalizer to set a bit telling us when
>>>> finalization has occurred. At this point we can assert that the
>>>> weak ref
>>>> has been cleared. In addition to being somewhat roundabout, this also
>>>> seems like we're merely cross-checking the garbage collector. We could
>>>> easily poll for the weak ref being cleared, and then assert that
>>>> the bit
>>>> set by finalization has indeed been set.
>>>>
>>>> It seems to me we could avoid finalization entirely and just use weak
>>>> refs and reference queues. Would it have the same effect and semantics
>>>> as the current test if we were to do the following?
>>>>
>>>> - with a target object, make a weak reference registered with a ref
>>>> queue
>>>> - clear all references to the target object
>>>> - request gc
>>>> - request finalization (is this necessary?)
>>>> - wait or poll on the ref queue
>>>>
>>>> If we get our weak ref from the queue, success. If we time out, fail.
>>>> Note that there is a timeout mechanism ReferenceQueue.remove(timeout),
>>>> so we could set some timeout without waiting the full jtreg timeout if
>>>> we don't want to. Or we could call System.gc() in a loop using
>>>> remove()
>>>> with a short timeout (instead of sleep), similar to the current test,
>>>> although it's not clear to me this is necessary.
>>>>
>>>> s'marks
>>>>
>>>>
>>>> On 6/27/12 1:20 AM, Eric Wang wrote:
>>>>> Hi David,
>>>>>
>>>>> Thank you for review!
>>>>>
>>>>> Hi Stuart,
>>>>>
>>>>> Can you please help to review and post the webrev, Thanks a lot!
>>>>>
>>>>> Eric
>>>>>
>>>>> On 2012/6/27 15:32, David Holmes wrote:
>>>>>> On 27/06/2012 4:57 PM, Eric Wang wrote:
>>>>>>> Hi David & Stuart,
>>>>>>>
>>>>>>> Thank you for the help! please review the in webrev 6948101.zip in
>>>>>>> attachment.
>>>>>>
>>>>>> Thanks Eric. That fix also seems fine to me.
>>>>>>
>>>>>> David
>>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> Eric
>>>>>>>
>>>>>>> On 2012/6/27 9:14, Stuart Marks wrote:
>>>>>>>> Hi Eric,
>>>>>>>>
>>>>>>>> Alan Bateman asked me to help you out with this since he's going
>>>>>>>> to be
>>>>>>>> unavailable for a couple weeks.
>>>>>>>>
>>>>>>>> I didn't see you on the OpenJDK census [1] so you might not
>>>>>>>> have an
>>>>>>>> Author role and thus the ability to post webrevs. If indeed you
>>>>>>>> don't,
>>>>>>>> I can post a webrev for you. I can also post a webrev for your
>>>>>>>> other
>>>>>>>> review (7123972) if it's still necessary.
>>>>>>>>
>>>>>>>> Finally, I can push the fixes for you when the reviews are
>>>>>>>> complete.
>>>>>>>>
>>>>>>>> s'marks
>>>>>>>>
>>>>>>>> [1] http://openjdk.java.net/census
>>>>>>>>
>>>>>>>> On 6/26/12 2:56 PM, David Holmes wrote:
>>>>>>>>> Hi Eric,
>>>>>>>>>
>>>>>>>>> On 26/06/2012 7:26 PM, Eric Wang wrote:
>>>>>>>>>> Please help to review the fix attached for test bug 6948101
>>>>>>>>>> <http://monaco.us.oracle.com/detail.jsf?cr=6948101> which is
>>>>>>>>>> same
>>>>>>>>>> root
>>>>>>>>>> cause as bug 7123972
>>>>>>>>>> <http://monaco.us.oracle.com/detail.jsf?cr=7123972>.
>>>>>>>>>> The test makes wrong assumption that GC is started
>>>>>>>>>> immediately to
>>>>>>>>>> recycle unused objects after System.gc() called.
>>>>>>>>>> The proposed fix is to make sure objects have been recycled
>>>>>>>>>> by GC
>>>>>>>>>> before
>>>>>>>>>> checking if the weak reference is null.
>>>>>>>>>
>>>>>>>>> Again I really need to see a webrev to see the fix in context.
>>>>>>>>> Do you
>>>>>>>>> have
>>>>>>>>> Author role and an OpenJDK user name so you can post webrevs on
>>>>>>>>> cr.openjdk.java.net?
>>>>>>>>>
>>>>>>>>> I suspect this may have the same issues as the other fix and
>>>>>>>>> require
>>>>>>>>> a timeout
>>>>>>>>> for when the original problem is still present.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Eric
>>>>>>>
>>>>>
>>
-------------- next part --------------
--- old/test/ProblemList.txt 2012-06-29 16:22:24.447967771 +0800
+++ new/test/ProblemList.txt 2012-06-29 16:22:22.518924402 +0800
@@ -271,9 +271,6 @@
# 7140992
java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java generic-all
-# 6948101
-java/rmi/transport/pinLastArguments/PinLastArguments.java generic-all
-
# 7146541
java/rmi/transport/rapidExportUnexport/RapidExportUnexport.java linux-all
-------------- next part --------------
--- old/test/java/rmi/transport/pinLastArguments/PinLastArguments.java 2012-06-29 16:22:29.171886035 +0800
+++ new/test/java/rmi/transport/pinLastArguments/PinLastArguments.java 2012-06-29 16:22:28.092898950 +0800
@@ -78,7 +78,13 @@
}
impl = null;
- System.gc();
+ while(true) {
+ System.gc();
+ Thread.sleep(20);
+ if(ref.get() == null) {
+ break;
+ }
+ }
if (ref.get() != null) {
throw new Error("TEST FAILED: impl not garbage collected");
More information about the core-libs-dev
mailing list