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.ri...@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
