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

[Roger Riggs]

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