[PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently

[Stuart Marks]

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
>>>
>