ThreadPoolExecutor and finalization

classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

ThreadPoolExecutor and finalization

roger riggs
Hi,

With the deprecation of Object.finalize its time to look at its uses too
see if
they can be removed or mitigated.

The ThreadPoolExecutor overrides finalize to shutdown the pool.
A simple but incomplete step is to retain the shutdown on finalize but
remove it from
the specification of ThreadPoolExecutor.finalize (and remove the
override).  [1]

For those familiar with Executors please take as look at the situation and
make a recommendation or suggestion.

Thanks, Roger

[1]  https://bugs.openjdk.java.net/browse/JDK-8190324




Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
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?

Thanks,
David

>
> The ThreadPoolExecutor overrides finalize to shutdown the pool.
> A simple but incomplete step is to retain the shutdown on finalize but
> remove it from
> the specification of ThreadPoolExecutor.finalize (and remove the
> override).  [1]
>
> For those familiar with Executors please take as look at the situation and
> make a recommendation or suggestion.
>
> Thanks, Roger
>
> [1]  https://bugs.openjdk.java.net/browse/JDK-8190324
>
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Andrej Golovnin-2
Hi David,

> On 30. Oct 2017, at 01:40, David Holmes <[hidden email]> 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.

Best regards
Andrej Golovnin
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
Hi Andrej,

On 30/10/2017 5:02 PM, Andrej Golovnin wrote:

> Hi David,
>
>> On 30. Oct 2017, at 01:40, David Holmes <[hidden email]> 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.

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?

Cheers,
David

> Best regards
> Andrej Golovnin
>
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

roger riggs
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 <[hidden email]>
>>> 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.
>
> 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.

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.

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.

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

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David M. Lloyd-3
On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <[hidden email]> wrote:
> ThreadPoolExecutor has a responsibility to cleanup any native resources it
> has allocated (threads) and it should be free to use whatever mechanism is appropriate.

I wonder though whether TPE.finalize() is ever actually hit in
practice: TPE is (by definition) a thread pool, and every live thread
in that pool has (by way of TPE.Worker) a strong reference to the TPE
itself via an outer class reference which is passed around in enough
places to make me think that it would never actually be collectable in
a real-world situation, unless all core threads were allowed to time
out.

--
- DML
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

roger riggs
Hi,

There is a test
<repo>/test/jdk/java/util/concurrent/Executors/AutoShutdown.java

It only tests it for Executors of newSingleThreadExecutor, not for the
others
so I wondered if there was some open issue.

Roger


On 10/30/2017 11:02 AM, David Lloyd wrote:

> On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <[hidden email]> wrote:
>> ThreadPoolExecutor has a responsibility to cleanup any native resources it
>> has allocated (threads) and it should be free to use whatever mechanism is appropriate.
> I wonder though whether TPE.finalize() is ever actually hit in
> practice: TPE is (by definition) a thread pool, and every live thread
> in that pool has (by way of TPE.Worker) a strong reference to the TPE
> itself via an outer class reference which is passed around in enough
> places to make me think that it would never actually be collectable in
> a real-world situation, unless all core threads were allowed to time
> out.
>

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Martin Buchholz-3
On Mon, Oct 30, 2017 at 8:27 AM, Roger Riggs <[hidden email]> wrote:

> Hi,
>
> There is a test <repo>/test/jdk/java/util/conc
> urrent/Executors/AutoShutdown.java
>
> It only tests it for Executors of newSingleThreadExecutor, not for the
> others
> so I wondered if there was some open issue.
>

I wrote that test long ago.  But I've always been confused about automatic
thread pool shutdown; it's not very reliable, especially given that all the
threads need to terminate.  Should TPE's finalization spec apply to STPE?

TPE is a brittle class; we avoid doing too much surgery on it (even though
we're in the middle of doing exactly that to fix an actual bug).

> The initiative is to identify and remediate existing uses of finalization
in the JDK.

I've been skeptical about this initiative as stated.  I would not have
deprecated finalize(). We will never remove finalize() from the JDK, and I
don't see how switching TPE from finalize to some other mechanism such as
Cleaner has real benefits for users.  There aren't enough instances of TPE
created for finalization to be a real user performance problem.

TPE's spec currently has a finalize deprecation warning, but this is not
helpful for users.
(a documentation readability regression!)
https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html#finalize--
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
In reply to this post by David M. Lloyd-3
On 31/10/2017 1:02 AM, David Lloyd wrote:

> On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <[hidden email]> wrote:
>> ThreadPoolExecutor has a responsibility to cleanup any native resources it
>> has allocated (threads) and it should be free to use whatever mechanism is appropriate.
>
> I wonder though whether TPE.finalize() is ever actually hit in
> practice: TPE is (by definition) a thread pool, and every live thread
> in that pool has (by way of TPE.Worker) a strong reference to the TPE
> itself via an outer class reference which is passed around in enough
> places to make me think that it would never actually be collectable in
> a real-world situation, unless all core threads were allowed to time
> out.

The docs for TPE cover this in detail: [1]

Finalization
     A pool that is no longer referenced in a program AND has no
remaining threads will be shutdown automatically. If you would like to
ensure that unreferenced pools are reclaimed even if users forget to
call shutdown(), then you must arrange that unused threads eventually
die, by setting appropriate keep-alive times, using a lower bound of
zero core threads and/or setting allowCoreThreadTimeOut(boolean).

David
-----

[1]
https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
In reply to this post by roger riggs
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 <[hidden email]>
>>>> 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.
>>
>> 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".

> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Peter Levart
In reply to this post by David Holmes
Hi,

On 10/31/17 05:12, David Holmes wrote:

> On 31/10/2017 1:02 AM, David Lloyd wrote:
>> On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <[hidden email]>
>> wrote:
>>> ThreadPoolExecutor has a responsibility to cleanup any native
>>> resources it
>>> has allocated (threads) and it should be free to use whatever
>>> mechanism is appropriate.
>>
>> I wonder though whether TPE.finalize() is ever actually hit in
>> practice: TPE is (by definition) a thread pool, and every live thread
>> in that pool has (by way of TPE.Worker) a strong reference to the TPE
>> itself via an outer class reference which is passed around in enough
>> places to make me think that it would never actually be collectable in
>> a real-world situation, unless all core threads were allowed to time
>> out.
>
> The docs for TPE cover this in detail: [1]
>
> Finalization
>     A pool that is no longer referenced in a program AND has no
> remaining threads will be shutdown automatically. If you would like to
> ensure that unreferenced pools are reclaimed even if users forget to
> call shutdown(), then you must arrange that unused threads eventually
> die, by setting appropriate keep-alive times, using a lower bound of
> zero core threads and/or setting allowCoreThreadTimeOut(boolean).

I'm trying to understand the purpose of finalize() in TPE, but can't.
I'm surely missing something. If the pool is no longer referenced AND
there are no active threads, what is there left to shutdown() actually?
All that remains is garbage that will eventually be GCed.

Regards, Peter

>
> David
> -----
>
> [1]
> https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html
>

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
On 31/10/2017 5:36 PM, Peter Levart wrote:

> On 10/31/17 05:12, David Holmes wrote:
>> On 31/10/2017 1:02 AM, David Lloyd wrote:
>>> On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <[hidden email]>
>>> wrote:
>>>> ThreadPoolExecutor has a responsibility to cleanup any native
>>>> resources it
>>>> has allocated (threads) and it should be free to use whatever
>>>> mechanism is appropriate.
>>>
>>> I wonder though whether TPE.finalize() is ever actually hit in
>>> practice: TPE is (by definition) a thread pool, and every live thread
>>> in that pool has (by way of TPE.Worker) a strong reference to the TPE
>>> itself via an outer class reference which is passed around in enough
>>> places to make me think that it would never actually be collectable in
>>> a real-world situation, unless all core threads were allowed to time
>>> out.
>>
>> The docs for TPE cover this in detail: [1]
>>
>> Finalization
>>     A pool that is no longer referenced in a program AND has no
>> remaining threads will be shutdown automatically. If you would like to
>> ensure that unreferenced pools are reclaimed even if users forget to
>> call shutdown(), then you must arrange that unused threads eventually
>> die, by setting appropriate keep-alive times, using a lower bound of
>> zero core threads and/or setting allowCoreThreadTimeOut(boolean).
>
> I'm trying to understand the purpose of finalize() in TPE, but can't.
> I'm surely missing something. If the pool is no longer referenced AND
> there are no active threads, what is there left to shutdown() actually?
> All that remains is garbage that will eventually be GCed.

Ummmmm .... I'm going to have to do some archaeology here ...

David

> Regards, Peter
>
>>
>> David
>> -----
>>
>> [1]
>> https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Peter Levart
Hi David,

On 10/31/17 08:45, David Holmes wrote:

>>> The docs for TPE cover this in detail: [1]
>>>
>>> Finalization
>>>     A pool that is no longer referenced in a program AND has no
>>> remaining threads will be shutdown automatically. If you would like
>>> to ensure that unreferenced pools are reclaimed even if users forget
>>> to call shutdown(), then you must arrange that unused threads
>>> eventually die, by setting appropriate keep-alive times, using a
>>> lower bound of zero core threads and/or setting
>>> allowCoreThreadTimeOut(boolean).
>>
>> I'm trying to understand the purpose of finalize() in TPE, but can't.
>> I'm surely missing something. If the pool is no longer referenced AND
>> there are no active threads, what is there left to shutdown()
>> actually? All that remains is garbage that will eventually be GCed.
>
> Ummmmm .... I'm going to have to do some archaeology here ...
>
> David

I can imagine a thread pool where worker threads, while idling, don't
have a reference to the pool so the pool can shutdown() itself when:

- the pool is no longer referenced AND
- there is no worker thread executing a task (i.e. all worker threads
are idle)

In such state, the pool is not reachable and may be shutdown.

But it seems that TPE is not such a pool.

Regards, Peter

>
>> Regards, Peter
>>
>>>
>>> David
>>> -----
>>>
>>> [1]
>>> https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html 
>>

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

David Holmes
Hi Peter,

cc'ing Doug Lea

On 31/10/2017 6:25 PM, Peter Levart wrote:

> Hi David,
>
> On 10/31/17 08:45, David Holmes wrote:
>>>> The docs for TPE cover this in detail: [1]
>>>>
>>>> Finalization
>>>>     A pool that is no longer referenced in a program AND has no
>>>> remaining threads will be shutdown automatically. If you would like
>>>> to ensure that unreferenced pools are reclaimed even if users forget
>>>> to call shutdown(), then you must arrange that unused threads
>>>> eventually die, by setting appropriate keep-alive times, using a
>>>> lower bound of zero core threads and/or setting
>>>> allowCoreThreadTimeOut(boolean).
>>>
>>> I'm trying to understand the purpose of finalize() in TPE, but can't.
>>> I'm surely missing something. If the pool is no longer referenced AND
>>> there are no active threads, what is there left to shutdown()
>>> actually? All that remains is garbage that will eventually be GCed.
>>
>> Ummmmm .... I'm going to have to do some archaeology here ...
>>
>> David
>
> I can imagine a thread pool where worker threads, while idling, don't
> have a reference to the pool so the pool can shutdown() itself when:
>
> - the pool is no longer referenced AND
> - there is no worker thread executing a task (i.e. all worker threads
> are idle)
>
> In such state, the pool is not reachable and may be shutdown.
>
> But it seems that TPE is not such a pool.

Right. The TPE has a set of Workers (nested class) and each Worker has a
Thread. The Thread executes the code of the Worker, so unless we hit the
classic finalize problem of "this" being elided in a running method
allowing the current object to get collected, then in theory any live
worker threads should keep the whole TPE reachable.

In 2006 we added the docs about it being unreferenced and no remaining
threads - which I guess is a necessary condition for finalization to now
occur. But as you noted at that point shutdown() is pretty much a no-op.

Maybe Doug (cc'd) can recall the gory details. :)

Cheers,
David

> Regards, Peter
>
>>
>>> Regards, Peter
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>> [1]
>>>> https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html 
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Peter Levart
In reply to this post by David Holmes
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 <[hidden email]>
>>>>> 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
>>

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Doug Lea
In reply to this post by David Holmes
On 10/31/2017 04:55 AM, David Holmes wrote:
>  
> In 2006 we added the docs about it being unreferenced and no remaining
> threads - which I guess is a necessary condition for finalization to now
> occur. But as you noted at that point shutdown() is pretty much a no-op.
>
> Maybe Doug (cc'd) can recall the gory details. :)
>
We added the wording to deal with complaints that pools were not
being auto-shutdown. But we didn't notice that the conditions for
finalization to trigger were the same as the conditions for
finalization not being necessary! (Just GCing would be fine.)

The best solution seems to be just to remove the method, and
adjust the javadoc wording to refer to GC vs finalization.

I think this would be binary compatible?

-Doug
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

roger riggs
In reply to this post by David Holmes
Hi David,

On 10/31/2017 12:37 AM, 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 <[hidden email]>
>>>>> 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.
>>>
>>> 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".
>
>> 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.
Not cited as authoritative, just one data point.
>
> 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.
In all the years of hand wringing over finalize no perfect replacement
has been discovered.
Deprecating and providing some advice, though not perfect for every
situation is a way
to get people thinking about alternatives that work in various cases.

>
> 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).
The first step is to deal with our own uses with in the JDK and try out
some workable alternatives
without breaking everything all at once.  It is expected to take a while
to mitigate all the uses.
In the meantime, reducing the uses and overhead due to finalization
gives a performance benefit.

Roger

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

roger riggs
In reply to this post by Peter Levart
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 .

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 <[hidden email]>
>>>>>> 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
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Jason Mehrens
In reply to this post by Peter Levart
Hi Peter,

Most of the discussion is in: https://bugs.openjdk.java.net/browse/JDK-6399443.  The linked issue in that report then links to the CI mail thread.

Jason


>I'm trying to understand the purpose of finalize() in TPE, but can't.
>I'm surely missing something. If the pool is no longer referenced AND
>there are no active threads, what is there left to shutdown() actually?
>All that remains is garbage that will eventually be GCed.

>Regards, Peter


Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor and finalization

Stuart Marks
In reply to this post by Martin Buchholz-3


On 10/30/17 10:21 AM, Martin Buchholz wrote:
>> The initiative is to identify and remediate existing uses of finalization
>> in the JDK.
>
> I've been skeptical about this initiative as stated.  I would not have
> deprecated finalize(). We will never remove finalize() from the JDK, and I
> don't see how switching TPE from finalize to some other mechanism such as
> Cleaner has real benefits for users.  There aren't enough instances of TPE
> created for finalization to be a real user performance problem.

Interesting that you say "we will never remove finalize()" ... it is exactly the
goal of this initiative to remove finalize() eventually. Or at least to remove
the finalization mechanism. It's been a thorn in the side of GC implementors
since forever. As Roger stated, the early part of this effort is to remove uses
from within the JDK, and to warn external users to start migrating to other
facilities. Hence, we've deprecated it and are having this discussion.

I don't know what the later parts of the transition will look like. Perhaps at
some point we deprecate Object.finalize() for removal; perhaps at some point the
VM stops calling Object.finalize() even though the method is declared; perhaps
at some point we actually remove the Object.finalize() method. All of this will
require further discussion, and it will be based on our experience working
through these early remediation steps.

> TPE's spec currently has a finalize deprecation warning, but this is not
> helpful for users.
> (a documentation readability regression!)
> https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ThreadPoolExecutor.html#finalize--

I'm not sure why you say this isn't helpful. It's clearly not helpful to
*clients* of TPE; but since finalize() is protected, the warning is clearly
directed at subclasses, and it provides information about migrating away from
finalization. Should say something different?

s'marks
12