On 11/01/17 02:49, David Holmes wrote:
Hi Roger,
On 31/10/2017 11:58 PM, Roger Riggs wrote:
Hi Peter,
Only native resources that do not map to the heap allocation/gc cycle
need any kind
of cleanup. I would work toward a model that encapsulates the
reference to a native resource
with a corresponding allocation/release mechanism as you've described
here and in the
thread on zip.
For cleanup purposes, the independence of each resource may improve
robustness
by avoiding dependencies and opportunities for entanglements and bugs
due to exceptions
and other failures.
In the case of TPE, the native resources are Threads, which keep
running even if they are
unreferenced and are kept referenced via ThreadGroups.
I don't know the Executor code well enough to do more than speculate,
but would suggest
that a cleaner (or similar) should be registered for each thread .
Threads are not native resources to be managed by Cleaners! A live
Thread can never be cleaned. A dead thread has nothing to clean!
Right, but an idle thread, waiting for a task that will never come since
the only entry point for submitting tasks is not reachable (the pool),
may be cleaned...
Regards, Peter
David
-----
For TPE, since Threads do not become unreferenced, the part of the
spec related to finalize
being called by the finalizer thread is moot.
$.02, Roger
On 10/31/2017 5:24 AM, Peter Levart wrote:
Hi,
Here are some of my thoughts...
On 10/31/17 05:37, David Holmes wrote:
Hi Roger,
On 31/10/2017 12:43 AM, Roger Riggs wrote:
Hi David,
On 10/30/2017 3:31 AM, David Holmes wrote:
Hi Andrej,
On 30/10/2017 5:02 PM, Andrej Golovnin wrote:
Hi David,
On 30. Oct 2017, at 01:40, David Holmes
<david.hol...@oracle.com> wrote:
Hi Roger,
On 30/10/2017 10:24 AM, Roger Riggs wrote:
Hi,
With the deprecation of Object.finalize its time to look at
its uses too see if they can be removed or mitigated.
So the nice thing about finalize was that it followed a
nice/clean/simple OO model where a subclass could override, add
their own cleanup and then call super.finalize(). With
finalize() deprecated, and the new mechanism being Cleaners,
how do Cleaners support such usages?
Instead of ThreadPoolExecutor.finalize you can override
ThreadPoolExecutor.terminated.
True. Though overriding shutdown() would be the semantic
equivalent of overriding finalize(). :)
In the general case though finalize() might be invoking a final
method.
Overriding shutdown() would only work when this method is explicitly
invoked by user code. Using Cleaner API instead of finalization can
not employ methods on object that is being tracked as that object is
already gone when Cleaner invokes the cleanup function.
Migrating TPE to Cleaner API might be particularly painful since the
state needed to perform the shutdown is currently in the TPE
instance fields and would have to be moved into the cleanup
function. The easiest way to do that is to:
- rename class ThreadPoolExecutor to package-private
ThreadPoolExecutorImpl
- create new class ThreadPoolExecutor with same public API
delegating the functionality to the embedded implementation
- registering Cleaner.Cleanable to perform shutdown of embedded
executor when the public-facing executor becomes phantom reachable
This would have an added benefit of automatically shut(ing)down()
the pool when it is not reachable any more but still has idle threads.
Anyway I'm not sure we can actually do something to try to move
away from use of finalize() in TPE. finalize() is only deprecated
- it is still expected to work as it has always done. Existing
subclasses that override finalize() must continue to work until
some point where we say finalize() is not only deprecated but
obsoleted (it no longer does anything). So until then is there
actually any point in doing anything? Does having a Cleaner and a
finalize() method make sense? Does it aid in the transition?
As you observe, the alternatives directly using PhantomRefs or the
Cleanup do not provide
as nice a model. Unfortunately, that model has been recognized to
have a number of
issues [1]. Finalization is a poor substitute for explicit
shutdown, it is unpredictable and unreliable,
and not guaranteed to be executed.
I'm not trying to support/defend finalize(). But it does support
the very common OO pattern of "override to specialize then call
super".
I have been thinking about that and I see two approaches to enable
subclasses to contribute cleanup code:
- one simple approach is for each subclass to use it's own
independent Cleaner/Cleanable. The benefit is that each subclass is
independent of its super/sub classes, the drawback is that cleanup
functions are called in arbitrary order, even in different threads.
But for cleaning-up independent resources this should work.
- the other approach is more involving as it requires the base class
to establish an infrastructure for contributing cleanup code. For
this purpose, the internal low-level Cleaner API is most appropriate
thought it can be done with public API too. For example (with public
API):
public class BaseClass implements AutoCloseable {
protected static class BaseResource implements Runnable {
@Override
public void run() {
// cleanup
}
}
protected final BaseResource resource = createResource();
private final Cleaner.Cleanable cleanable =
CleanerFactory.cleaner().register(this, resource);
protected BaseResource createResource() {
return new BaseResource();
}
public BaseClass() {
// init BaseResource resource...
}
@Override
public final void close() {
cleanable.clean();
}
}
public class DerivedClass extends BaseClass {
protected static class DerivedResource extends BaseResource {
@Override
public void run() {
// before-super cleanup
super.run();
// after-super cleanup
}
}
@Override
protected DerivedResource createResource() {
return new DerivedResource();
}
public DerivedClass() {
super();
DerivedResource resource = (DerivedResource) this.resource;
// init DerivedResource resource
}
}
And alternative with private low-level API:
public class BaseClass implements AutoCloseable {
protected static class BaseResource extends
PhantomCleanable<BaseClass> {
protected BaseResource(BaseClass referent) {
super(referent, CleanerFactory.cleaner());
}
@Override
protected void performCleanup() {
// cleanup
}
}
protected final BaseResource resource = createResource();
protected BaseResource createResource() {
return new BaseResource(this);
}
public BaseClass() {
// init BaseResource resource...
}
@Override
public final void close() {
resource.clean();
}
}
public class DerivedClass extends BaseClass {
protected static class DerivedResource extends BaseResource {
protected DerivedResource(DerivedClass referent) {
super(referent);
}
@Override
protected void performCleanup() {
// before-super cleanup
super.performCleanup();
// after-super cleanup
}
}
@Override
protected DerivedResource createResource() {
return new DerivedResource(this);
}
public DerivedClass() {
super();
DerivedResource resource = (DerivedResource) this.resource;
// init DerivedResource resource
}
}
Regards, Peter
ThreadPoolExecutor has a responsibility to cleanup any native
resources it has allocated (threads)
and it should be free to use whatever mechanism is appropriate.
Currently, the spec for finalize
does not give it that freedom.
I suppose in hindsight we (JSR-166 EG) could have described
automatic shutdown without mentioning the word "finalize", but that
ship has sailed. We did specify it and people can expect it to be
usable and they can expect it to work. While encouraging people to
move away from finalization is a good thing you have to give them a
workable replacement.
The initiative is to identify and remediate existing uses of
finalization in the JDK.
The primary concern is about subclasses that reply on the current
spec.
If I'm using grepcode correctly[2], it does not show any
subclasses of ThreadPoolExecutor that
override finalize; so it may be a non-issue.
Pardon my skepticism if I don't consider "grepcode" as a reasonable
indicator of whether there are subclasses of TPE that override
finalize. Only a small fraction of source code is in the open.
And I have to agree with Martin that the current documentation for
finalize() in TPE is somewhat inappropriate. If you deprecate
something you're supposed to point the user to the preferred way of
doing something other than the deprecated method - but there is
none. finalize() in TPE should not have been deprecated until we
provided that alternative.
I think the way forward here would be to:
1. Change TPE.finalize() to final to force any subclasses to
migrate to the new mechanism going forward.
2. Implement the new mechanism - presumably Cleaner - and document
how to achieve the effect of "override to specialize then call
super" (presumably by overriding shutdown() instead).
then in a few releases we remove TPE.finalize() (at the same time
as we remove it from Object).
Cheers,
David
Regards, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8165641
[2]
http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@8u40-b25@java%24util%24concurrent@ThreadPoolExecutor@finalize%28%29&k=d
