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

[David Holmes]

On 30/06/2012 8:00 AM, Stuart Marks wrote:
> Thanks for updating this. I've posted the revised webrev:
>
> http://cr.openjdk.java.net/~smarks/yiming.wang/6948101/webrev.1/
>
> It's good that this is getting simpler in that we don't have to deal
> with finalization. But it can still get simpler.... The code is in the
> webrev but I'll also paste it here for convenience:
>
> 81 while(true) {
> 82 System.gc();
> 83 Thread.sleep(20);
> 84 if(ref.get() == null) {
> 85 break;
> 86 }
> 87 }
> 88
> 89 if (ref.get() != null) {
> 90 throw new Error("TEST FAILED: impl not garbage collected");
> 91 }
>
> The only way we the loop can terminate normally is if ref.get() returns
> null, so it's pointless to test ref.get() again after the loop. I'd just
> take out this second test of ref.get().

Agreed. The test can no longer "fail", it either completes or hangs. So 
lines 89/90 are useless in terms of testing what this test is supposed 
to be testing.

> Now, how can the test fail? If ref is never cleared, the while loop will
> never terminate, and we rely on jtreg to timeout and kill this test. It
> would be good to have a brief comment above the while loop that explains
> this.

Agreed. Something like

// Might require multiple calls to System.gc() for weak-references 
processing to be complete.
// If the weak-reference is not cleared as expected we will hang here
// until timed out by the test harness

Though now I write that I'm unhappy that this will only behave nicely if 
run from jtreg. We do use explicit timeouts in other tests.

David
-----


> Finally, a style nitpick: there should be a space before the ( for the
> while and if statements. That's the prevailing style we use in the JDK.
>
> s'marks
>
>
> On 6/29/12 1:31 AM, Eric Wang wrote:
>> 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
>>>>>>>>>
>>>>>>>
>>>>
>>