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