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

[David Holmes]

Hi Roger,

Sorry to be late here but was trying not to get involved :)

It is already implicit that ThreadFactory.newThread should return 
unstarted threads - that is what a new Thread is - so I don't think 
IllegalThreadStateException needs to be documented here as it is 
documenting behaviour of a broken ThreadFactory (and a broken 
ThreadFactory could throw anything) .

Also the no-arg cleaner() can also throw SecurityException.

Also this:

127      * On each call the {@link ThreadFactory#newThread(Runnable) 
thread factory}
  128      * should return a {@link Thread.State#NEW new thread} with an 
appropriate
  129      * {@linkplain Thread#getContextClassLoader context class loader},
  130      * {@linkplain Thread#getName() name},
  131      * {@linkplain Thread#getPriority() priority},
  132      * permissions, etc.

then begs the questions as to what is "appropriate". I think this can be 
said much more simply as: "The ThreadFactory must provide a Thread that 
is suitable for performing the cleaning work". Though even that raises 
questions. I'm not sure why a ThreadFactory is actually needed here ?? 
Special security context? If so that could be mentioned, but I don't 
think name or priority need to be discussed.

Thanks,
David

On 6/12/2015 3:26 AM, Roger Riggs wrote:
> 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
>
>