[PATCH] Review Request - Test bug: 6948101 java/rmi/transport/pinLastArguments/PinLastArguments.java failing intermittently
Stuart Marks
stuart.marks at oracle.com
Fri Jun 29 22:00:16 UTC 2012
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().
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.
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
>>>>>>>>
>>>>>>
>>>
>
More information about the core-libs-dev
mailing list