RFR 9: 8146012: CleanerTest fails: Cleanable should have been freed

[Chris Hegarty]

Thanks for your reply Roger. I’m fine with the changes are they are.

-Chris.


On 22 Dec 2015, at 18:10, Roger Riggs <Roger.Riggs at Oracle.com> wrote:

> Hi Chris,
> 
> Thanks for the review
> 
> On 12/22/2015 12:01 PM, Chris Hegarty wrote:
>> Hi Roger,
>> 
>> On 22 Dec 2015, at 16:35, Roger Riggs 
>> <Roger.Riggs at oracle.com>
>>  wrote:
>> 
>> 
>>> Please review improvements to the CleanerTest to improve the reliability of the test.
>>> 
>>> Webrev:
>>>  
>>> http://cr.openjdk.java.net/~rriggs/webrev-cleanertest-8146012/
>> 
>> The use of WhiteBox should make the test more reliable ( rather than relying
>> on generating garbage ).
>> 
>> Is @library /lib/testlibrary  necessary? I don’t see any usage of types from it.
>> 
> yes, the ClassFileInstaller that enables the WhiteBox is there.
>> 
>> Is '-Xbootclasspath/a:.’ necessary? Is this for WhiteBox, or the test itself? I
>> don’t see why it is necessary.
>> 
> yes, That's how the share library for the whitebox native code is found.
>> 
>> Moving from a varargs of Semaphore to just a single Semaphore does simplify
>> the code, since it is always called with a single Semaphore.
>> 
>> The new checkCleaned does not enforce availablePermits == 1, just that a permit
>> is available. Should it assert 1 ?
>> 
> The tryAdvance method consumes the expected permit and the method does not wait around to
> see if it will get incremented again.  So checking again would be hit or miss.  The check completes
> when the semaphore is triggered.
> The running time of the test would increase and I can't see it paying off since the Cleaner 
> implementation is written to call it only once, so it would require some unexpected case to trigger that bug.
> 
> Roger
> 
>> 
>> -Chris.
>> 
>> 
>>> Issue:
>>>   
>>> https://bugs.openjdk.java.net/browse/JDK-8146012
>>> 
>>> 
>>> Thanks, Roger
>>> 
>