RFR 8135188: RunFinalizationTest.java Exception java.lang.Error: Test failure: Object was not finalized

[Daniel D. Daugherty]

On 10/13/15 2:43 AM, Jaroslav Bachorik wrote:
> On 12.10.2015 19:09, Daniel D. Daugherty wrote:
>>  > http://cr.openjdk.java.net/~jbachorik/8135188/webrev.02
>>
>> test/serviceability/dcmd/gc/RunFinalizationTest.java
>>      No comments.
>>
>> test/serviceability/dcmd/gc/FinalizationRunner.java
>>      L58:         o = new MyObject();
>>      L59:         o = null;
>>
>>      L79:         o = new MyObject();
>>      L80:         o = null;
>>          So now two different threads are initializing this static 
>> field:
>>
>>          55     public static MyObject o;
>>
>>          and both are clearing it. Is that just a left over
>>          from simplifying the test?
>
> Actually, this is needed for the successful test run. On L79-80 the 
> instance is made eligible for finalization so we can block the regular 
> finalizer thread and make sure that the instance from L58-59 is 
> finalized due to GC.run_finalization.
>
> I've updated the test to make the test intentions clear - added more 
> comments and debug output.
>
> http://cr.openjdk.java.net/~jbachorik/8135188/webrev.03

test/serviceability/dcmd/gc/RunFinalizationTest.java
     No comments.

test/serviceability/dcmd/gc/FinalizationRunner.java
     L42: System.out.println("inisde the regular finalizer thread; 
blocking");
         Typo: 'inisde' -> 'inside'

     L61:     /* this instance will be used to provoke the regular 
finalization
     L62:        so the finalizer thread can be blocked for the duration of
     L63:        GC.run_finalization test */
         Please switch to '//' style comments for this block.

     L66:     /* this instance will be used to perform the 
GC.run_finalization test */
         Please switch to '//' style comment here.

I don't need to see a new webrev for the above changes.

Thanks for the refactor and the new comments. Your intent
is much more clear now.

Thumbs up.

Dan


>
> Thanks,
>
> -JB-
>
>>
>> Dan
>>
>>
>>
>> On 10/12/15 2:00 AM, Jaroslav Bachorik wrote:
>>> On 9.10.2015 20:05, Martin Buchholz wrote:
>>>>
>>>>
>>>> On Thu, Oct 8, 2015 at 11:51 PM, Jaroslav Bachorik
>>>> <jaroslav.bachorik at oracle.com <mailto:jaroslav.bachorik at oracle.com>>
>>>> wrote:
>>>>
>>>>     On 8.10.2015 18:56, Martin Buchholz wrote:
>>>>
>>>>         Hi Jaroslav,
>>>>
>>>>         we all keep writing finalization code like this... welcome to
>>>>         the club!
>>>>
>>>>             I think it would be better :
>>>>         - never use currentTimeMillis to measure elapsed time; use
>>>>         nanoTime instead
>>>>
>>>>
>>>>     Ok. I suppose this would be because currentTimeMillis() is 
>>>> dependent
>>>>     on the OS time, right?
>>>>
>>>>         - why use complex Phaser when simple CountDownLatch will do?
>>>>
>>>>
>>>>     The logic is more complex than just waiting for the 
>>>> finalization to
>>>>     happen. I need to make sure the finalization happened due to
>>>>     GC.run_finalization command and not because of an ordinary GC 
>>>> run or
>>>>     JVM shutdown. I will update the test comments to make this clear.
>>>>
>>>>
>>>> Oh, now I see what you're doing - you need to block the regular
>>>> finalizer thread to make sure there will be objects available for the
>>>> secondary finalizer thread to process.  Although Phaser works for 
>>>> this,
>>>> I like using simple latches - CountDownLatch(1) - because they are
>>>> easier to understand.
>>>>
>>>> CountDownLatch done = new CountDownLatch(1);
>>>>
>>>> in primary finalizer thread, call done.await
>>>> in secondary finalizer thread, call done.countDown to release the
>>>> primary finalizer thread
>>>
>>> Ok, I took a look at the test from distance and simplified it a bit.
>>> Did a test run of 500 iterations in tight loop without failure.
>>>
>>> http://cr.openjdk.java.net/~jbachorik/8135188/webrev.02
>>>
>>> -JB-
>>
>