ThreadPoolExecutor and finalization
Peter Levart
peter.levart at gmail.com
Wed Nov 1 12:20:51 UTC 2017
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...
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