On 12/04/2015 10:04 PM, Roger Riggs wrote:
Hi Peter,
On 12/04/2015 03:30 PM, Peter Levart wrote:
Just a nit more, Roger:
131 * <p>
132 * The cleaner terminates when it is unreachable and all of the
133 * registered cleaning functions are complete.
(and also in the javadoc of the other create() method)
The cleaner is an object. What terminates is a thread. So what about:
"The thread terminates after the cleaner becomes unreachable and all
of the registered cleaning functions have completed."
The cleaner is not just an object but is an active processing element.
The thread is an artifact of the implementation and not exposed in the API
any more than necessary (due to the thread factory).
Colloquially, the combination of state and thread can be referred to
as the cleaner;
it starts, it processes cleanup functions for unreachable objects, and
it stops when there are no more.
It may be useful to talk about the thread terminating, but that is an
implementation detail
and not readily visible.
Ok, I buy that.
Would writing something like the following make sense: "A future
implementation may use more than one thread. The ThreadFactory should
not assume that only one thread will be requested." ?
I'm not sure it adds anything to talk about how many times the thread
factory will be used.
A ThreadFactory has no specific mins or maxes.
Roger
Yes, but other parts of javadoc strictly mention a thread (as singular
object).
Users are sloppy and like to depend on implementation details. They will
be tempted to write code like this:
Thread t = new Thread(...);
t.setName(...);
...
Cleaner c = Cleaner.create(() -> t);
...and it will work. But will fail if some time in the future, Cleaner
decides to request multiple threads. Are you sure Cleaner will always
use a single thread? It will if the spec. says so. But I don't think
there is a reason to specify that and limit the implementation to one
thread forever.
Regards,
Peter
Regards, Peter
On 12/04/2015 05:55 PM, Roger Riggs wrote:
Hi,
Thanks for the review and comments.
The webrev[1] and javadoc[2] are updated in place.
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 4:50 PM, mark.reinh...@oracle.com wrote:
Looks good -- thanks for the further simplification.
Minor editorial comments, to add what Kim and Chris noted:
- In many places you write, e.g., "Cleaner" rather than "{@code
Cleaner}". For consistency with the rest of the package it'd be
better in most cases just to write "cleaner" or, if its nature as
a class is important, write "{@code Cleaner}". The same goes for
Cleanable, Thread, ThreadFactory, and all other types.
- The specification of Cleaner::create() mentions
"ThreadContextClassLoader", but that's not actually a type
anywhere
in the JDK. Suggest "{@linkplain
java.lang.Thread#getContextClassLoader context class loader}.
- In the same method, it'd be helpful to provide links into the
Thread
class (or wherever) for the concepts of access-control context
and
thread locals.
- Mark
