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

[Jaroslav Bachorik]

On 13.10.2015 15:32, Daniel D. Daugherty wrote:
> 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.

Thanks, Dan!

If I don't hear any more objections I will integrate tomorrow (CEST).

-JB-

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