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

[Daniel D. Daugherty]

 > http://cr.openjdk.java.net/~jbachorik/8135188/webrev.01

test/serviceability/dcmd/gc/RunFinalizationTest.java
     L53:         OutputAnalyzer out = 
ProcessTools.executeProcess(testAppPb);
     L54:         out.stderrShouldNotMatch("^" + 
FinalizationRunner.FAILED + ".*")
     L55:            .stdoutShouldMatch("^" + FinalizationRunner.PASSED 
+ ".*");

         What's the return value from out.stderrShouldNotMatch()?
         Does it simply return 'out'? Sorry, it's been a while
         since I read the test framework...

     No other comments.

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

Thumbs up.

Dan


On 10/9/15 1:36 AM, Jaroslav Bachorik wrote:
> On 9.10.2015 08:51, Jaroslav Bachorik 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.
>>
>>>
>>> I just concidentally wrote this code, which I hope will be a model of
>>> best practice:
>>>
>>>      /** No guarantees, but effective in practice. */
>>>      private static void forceFullGc() {
>>>          CountDownLatch finalizeDone = new CountDownLatch(1);
>>>          WeakReference<?> ref = new WeakReference<Object>(new 
>>> Object() {
>>>              protected void finalize() { finalizeDone.countDown(); }});
>>>          try {
>>>              for (int i = 0; i < 10; i++) {
>>>                  System.gc();
>>>                  if (finalizeDone.await(1L, SECONDS) && ref.get() ==
>>> null) {
>>>                      System.runFinalization(); // try to pick up
>>> stragglers
>>>                      return;
>>>                  }
>>>              }
>>>          } catch (InterruptedException unexpected) {
>>>              throw new AssertionError("unexpected 
>>> InterruptedException");
>>>          }
>>>          throw new AssertionError("failed to do a \"full\" gc");
>>>      }
>>
>> This seems to be testing only the fact that the finalization has
>> eventually been run - not that the finalization was actually provoked by
>> System.gc() or System.runFinalization(). Unfortunately, this wouldn't
>> suffice for my needs here.
>
> Updated webrev addressing Dan's and Martin's concerns - 
> http://cr.openjdk.java.net/~jbachorik/8135188/webrev.01
>
> -JB-
>
>>
>> -JB-
>>
>>>
>>>
>>> On Thu, Oct 8, 2015 at 12:37 AM, Jaroslav Bachorik
>>> <jaroslav.bachorik at oracle.com <mailto:jaroslav.bachorik at oracle.com>>
>>> wrote:
>>>
>>>     Please, review the following test change
>>>
>>>     Issue : https://bugs.openjdk.java.net/browse/JDK-8135188
>>>     Webrev: http://cr.openjdk.java.net/~jbachorik/8135188/webrev.00
>>>
>>>     The problem is described in detail in the issue. This patch follows
>>>     the recommendation by Dan Daugherty and runs the part where
>>>     finalization is being requested in a shutdown handler as a separate
>>>     process. The test then checks the result of this separate 
>>> process to
>>>     see whether any errors were reported. In order to simplify the 
>>> logic
>>>     of the shutdown handler the failures are reported by simply 
>>> printing
>>>     messages into stderr as opposed to throwing 'new Error()'.
>>>
>>>     The modified test is still passing on all supported platforms.
>>>
>>>     Thanks,
>>>
>>>     -JB-
>>>
>>>
>>
>