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

[Roger Riggs]

Hi Mandy,

Thanks for the comments,

Updated the artifacts:

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



On 12/04/2015 08:35 PM, Mandy Chung wrote:
>> On Dec 4, 2015, at 8:56 AM, Roger Riggs <roger.riggs at oracle.com> wrote:
>>
>>
>> [1] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
>> [2] http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
>    67  * Unless otherwise noted, passing a {@code null} argument to a constructor or
>    68  * method in any class or interface in this package will cause a
>
> s/in any class or interface in this package/in this class/
right
>
>   100      * Returns a new Cleaner.
> s/Cleaner/{@code Cleaner}
right
>
> For the create() factory method: should it prepare for future optimization (e.g. returning a shared Cleaner object, or multiple Cleaner object sharing among one or more dedicated threads)?   Would it be better not to require a new thread be created and also “Return a Cleaner” instead of “a new Cleaner”.

You will remember that a common cleaner method was proposed a couple of 
months ago
and it was removed because it was thought to be an attractive nuisance.
If any library or application can use it, then bugs, intentional or not 
can disrupt the cleaning
mechanism.  Such a cleaner would need to be written very defensively and 
be prepared for
cleaning functions to block or not return. There is no way to be fully 
robust if some code
is very badly behaved on a thread;  threads can not be stopped or 
destroyed effectively.
It was considered at that time that every library and application needed 
to give adequate
thought to its cleaning needs and implement accordingly.

If there is to be a common cleaner provided, it should be a different 
factory method that explicitly
returns a common cleaner.  For example,
     public static Cleaner commonCleaner() {...}

>
>   105      * of the thread is set to the system class loader.
>
>    - can you add a link to ClassLoader#getSystemClassLoader
ok  ClassLoader#getSystemClassLoader
>
>   106      * The thread has no permissions, enforced only if a
>   107      * {@link java.lang.System#setSecurityManager(SecurityManager) SecurityManager is set}.
>
> I realized I didn’t bring this up in my previous comment.  I meant to say that it may be fine not to mention about “inherited ACC” and permissions.  I included that sentence just in case there is a reason it needs.  I am not aware of any specification talking about the inherited ACC of a newly created thread.   This sentence can be removed.  Maybe you can add a @apiNote to explain it.
It should be clear and specified what access control is effective for 
the new thread.
Since InnocuousThread is non-public, it is uncomfortable to refer to it 
(as does ForkJoinWorkerThread)
Next best is to describe the attributes of the threads it produces.

>
>> Which brings up a gap in the specification of create() that it can throw exceptions related
>> to creation and starting of threads, including SecurityExceptions, IllegalThreadStateException, etc.
> Regarding the exceptions if threadFactory.newThread returns a started thread, @throws IllegalThreadStateException is a good one and I think that’s all.  A new thread implies “not started” - perhaps adding a link to Thread.State.NEW.  I did look at whether the security exception will be thrown and I don’t see any.  You should double check.
For the default threadFactory, there should be no exceptions, between a 
clean thread state
and doPrivileged to create the thread.

But for the create(ThreadFactory) method, SecurityExceptions could 
occur; and IllegalThreadStateException if it was started.
>
> Cleanable is missing @since 9
fixed
>
>   164      * Cleanable is a registered cleaning function.
>
> I read “thunk” passed to Cleaner::register is the “registered cleaning function”.  What about:
>
> {@code Cleanable} represents the object and its associated cleaning function registered in a {@code Cleaner}
a bit more concise and aligned with the description of register().

{@code Cleanable} represents an object and a cleaning function registered in a {@code Cleaner}.

The only function available through the Cleanable is the cleaning function.

Thanks, Roger