RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Roger Riggs]

Hi Mandy,

The webrev[1] and javadoc[2] are updated in to address your comments.

Thanks, Roger

[1] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
[2] http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html


On 12/3/2015 8:43 PM, Mandy Chung wrote:
>> On Dec 3, 2015, at 1:19 PM, Roger Riggs <roger.riggs at oracle.com> wrote:
>>
>> [1] http://cr.openjdk.java.net/~rriggs/cleaner-doc/
>> [2] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
> The implementation looks good.  I’ll wait for an updated javadoc and re-review that incorporates the editorial comments that Mark made.
>
> Some additional comments:
> - null should be {@code null}
>
>   150      * Refer to the API Note above for cautions about the behavior
>   151      * of cleaning functions.
ok
>
> It may be good to add a link back to the API note.
ok
>
> 76  * and not block. If the cleaning function blocks it may delay processing
> - should there be “,”  before “it…”
ok
>
>    77  * other cleaning functions using the same Cleaner.
>    78  * All cleaning functions using a Cleaner should be mutually compatible.
> - “using a cleaner” - should the terminology be “registered in a cleaner” consistent with other references in the spec?
ok
>
>   104      * The thread sets the ThreadContextClassLoader to the system class loader,
>   105      * sets the AccessControlContext to an empty ProtectionDomain,
>   106      * clears ThreadLocals, and sets the thread to be a
>   107      * {@link Thread#setDaemon(boolean) daemon thread}.
>
> I think clearing thread locals may not need to be mentioned.  Below is one suggestion to rephrase the above.  Similarly can be applied to the thread factory variant.
>
> * The thread is a {@linkplain Thread#isDaemon() daemon thread}.
> * The thread's {@linkplain Thread#getContextClassLoader context class loader}
> * is set to {@linkplain ClassLoader#getSystemClassLoader() system class loader}.
> * The thread has no permission to access any security operation.
Reworded both method doc.
>
> Minor comment on the CleanerTest.java:
>
>   530                     // unless it is know this case throws an exception, rethrow
>
> typo: s/know/known
>
> It would be better to rename the test[1-5] methods to reflect what it verifies.
done
>
>   254             Reference<?> r = queue.remove();
>
> Minor one: remove(long timeout) may be a better option while in practice remove() will not block forever.
done.

Roger

>
> Mandy