ThreadPoolExecutor and finalization
David Holmes
david.holmes at oracle.com
Wed Nov 1 01:49:54 UTC 2017
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!
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