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

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;

More information about the core-libs-dev mailing list