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