Hi Peter,
On 12/04/2015 04:31 PM, Peter Levart wrote:
...
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.
ok, how about:
* On each call the {@link ThreadFactory#newThread(Runnable) thread
factory} * should return a new thread, {@link Thread#start() not
started}, * with an appropriate * {@linkplain
Thread#getContextClassLoader context class loader}, * {@linkplain
Thread#getName() name}, * {@linkplain Thread#getPriority() priority}, *
permissions, etc.
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.
If a future cleaner implementation were to use multiple threads, it
would need to handle
exceptions from invoking ThreadFactory.newThread and be designed to cope
with them.
A successfully created cleaner will have at least one thread to work with.
Thanks, Roger
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
