ThreadPoolExecutor and finalization
David Holmes
david.holmes at oracle.com
Wed Nov 1 12:34:36 UTC 2017
On 1/11/2017 10:20 PM, Peter Levart wrote:
> On 11/01/17 10:04, David Holmes wrote:
>> On 1/11/2017 6:16 PM, Peter Levart wrote:
>>> 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...
>>
>> cleaned? It can be interrupted if you know about it and find locate
>> it. But it will not be eligible for cleaning ala Cleaner as it will
>> always be strongly reachable.
>
> Ah I see what you meant before. Yes, tracking Thread object with a
> Cleaner does not have any sense. But tracking thread pool object with a
> Cleaner and cleaning (stopping) threads as a result makes sense...
No, because live Threads will keep the thread pool strongly reachable.
If you manage to structure things such that the Threads don't keep the
pool strongly reachable then you risk having the pool cleaned while
still actively in use.
David
David
> Regards, Peter
>
>>
>> David
>>
>>> 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.holmes at 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
>>>>>>>>
>>>>>>
>>>>>
>>>
>
More information about the core-libs-dev
mailing list