ThreadPoolExecutor and finalization
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
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
Hi David,
On 30. Oct 2017, at 01:40, David Holmes <david.holmes@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. Best regards Andrej Golovnin
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@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. 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
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@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.
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%24j...
On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <Roger.Riggs@oracle.com> 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
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 <Roger.Riggs@oracle.com> 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.
On Mon, Oct 30, 2017 at 8:27 AM, Roger Riggs <Roger.Riggs@oracle.com> 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/ThreadPoolExe...
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/ThreadPoolExe...
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
Hi Stuart, Jumping to the end ... On 1/11/2017 6:37 AM, Stuart Marks wrote:
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/ThreadPoolExe...
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?
It isn't helpful to people subclassing TPE (or any class that defines finalize()) because the general strategies described in Object.finalize() can't be applied by the subclass independent of the class (TPE here) it is subclassing. Unless TPE provides an alternate mechanism, the subclass is stuck with finalize(). David
s'marks
On 10/31/17 6:58 PM, David Holmes wrote:
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?
It isn't helpful to people subclassing TPE (or any class that defines finalize()) because the general strategies described in Object.finalize() can't be applied by the subclass independent of the class (TPE here) it is subclassing. Unless TPE provides an alternate mechanism, the subclass is stuck with finalize().
I don't understand why the subclass has to rely on the mechanism provided by its superclass (TPE here). I'm thinking of a case like the following. Suppose a subclass has some independent native resource that it needs to clean up. Prior to the introduction of java.lang.ref, the only thing available was finalization. The subclass would have to override finalize(), do its own cleanup, and then make sure to call super.finalize(). With Cleaner in JDK 9, the subclass can refactor its native resources into a Cleanable, register it with a Cleaner, and then clean up its native resources when the Cleanable's clean() method is called. Can't this be done independently of TPE and finalization? s'marks
On 2/11/2017 5:07 AM, Stuart Marks wrote:
On 10/31/17 6:58 PM, David Holmes wrote:
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?
It isn't helpful to people subclassing TPE (or any class that defines finalize()) because the general strategies described in Object.finalize() can't be applied by the subclass independent of the class (TPE here) it is subclassing. Unless TPE provides an alternate mechanism, the subclass is stuck with finalize().
I don't understand why the subclass has to rely on the mechanism provided by its superclass (TPE here). I'm thinking of a case like the following.
Suppose a subclass has some independent native resource that it needs to clean up. Prior to the introduction of java.lang.ref, the only thing available was finalization. The subclass would have to override finalize(), do its own cleanup, and then make sure to call super.finalize().
With Cleaner in JDK 9, the subclass can refactor its native resources into a Cleanable, register it with a Cleaner, and then clean up its native resources when the Cleanable's clean() method is called.
Can't this be done independently of TPE and finalization?
Of course it can. The independent part of the subclass can use whatever mechanism it likes - it's independent. The point is that if the subclass needs to coordinate it's additional cleanup with the shutdown done by finalize() then it needs a means to do so. David
s'marks
Hi David, On 11/1/2017 10:30 PM, David Holmes wrote:
On 2/11/2017 5:07 AM, Stuart Marks wrote:
On 10/31/17 6:58 PM, David Holmes wrote:
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?
It isn't helpful to people subclassing TPE (or any class that defines finalize()) because the general strategies described in Object.finalize() can't be applied by the subclass independent of the class (TPE here) it is subclassing. Unless TPE provides an alternate mechanism, the subclass is stuck with finalize().
I don't understand why the subclass has to rely on the mechanism provided by its superclass (TPE here). I'm thinking of a case like the following.
Suppose a subclass has some independent native resource that it needs to clean up. Prior to the introduction of java.lang.ref, the only thing available was finalization. The subclass would have to override finalize(), do its own cleanup, and then make sure to call super.finalize().
With Cleaner in JDK 9, the subclass can refactor its native resources into a Cleanable, register it with a Cleaner, and then clean up its native resources when the Cleanable's clean() method is called.
Can't this be done independently of TPE and finalization?
Of course it can. The independent part of the subclass can use whatever mechanism it likes - it's independent. The point is that if the subclass needs to coordinate it's additional cleanup with the shutdown done by finalize() then it needs a means to do so. Given the use cases and the context in which an unreferenced resource needs to be released, I don't think a fully general purpose mechanism is appropriate. If there are dependencies in the cleanup those can be references between the dependent objects. Those dependencies will ensure that cleanup of each occurs when it is unreferenced. It is more robust if each resource handles its own cleanup. Peter already suggested that if coordination was required, the mechanism should be explicitly provided by the owning/coordinating object.
Roger
David
s'marks
On 31/10/2017 1:02 AM, David Lloyd wrote:
On Mon, Oct 30, 2017 at 9:43 AM, Roger Riggs <Roger.Riggs@oracle.com> 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/ThreadPoolExe...
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 <Roger.Riggs@oracle.com> 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/ThreadPoolExe...
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 <Roger.Riggs@oracle.com> 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/ThreadPoolExe...
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/ThreadPoolExe...
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/ThreadPoolExe...
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
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
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@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.
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%24j...
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@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%24j...
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 <david.holmes@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%24j...
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@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%24j...
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@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%24j...
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. 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@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%24j...
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@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%24j... >
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@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%24j... >>
On 11/01/17 13:34, David Holmes wrote:
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.
Pool is actively in use when it is still reachable. Only in that case can new tasks be submitted. When it is not reachable any more, no new tasks can be submitted and it can be shutdown(): /** * Initiates an orderly shutdown in which previously submitted * tasks are executed, but no new tasks will be accepted... Regards, Peter
On 2/11/2017 3:46 AM, Peter Levart wrote:
On 11/01/17 13:34, David Holmes wrote:
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.
Pool is actively in use when it is still reachable. Only in that case can new tasks be submitted. When it is not reachable any more, no new tasks can be submitted and it can be shutdown():
/** * Initiates an orderly shutdown in which previously submitted * tasks are executed, but no new tasks will be accepted...
Didn't we already determine that a Cleaner can't call shutdown() because that requires a strong reference it doesn't have? I think Doug already summed this up. The existing finalize() is pointless because when it could be called there is nothing left to be "cleaned up". There's no need for any use of Cleaner (even if it could do anything useful). David
Regards, Peter
On 11/02/2017 03:34 AM, David Holmes wrote:
On 2/11/2017 3:46 AM, Peter Levart wrote:
On 11/01/17 13:34, David Holmes wrote:
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.
Pool is actively in use when it is still reachable. Only in that case can new tasks be submitted. When it is not reachable any more, no new tasks can be submitted and it can be shutdown():
/** * Initiates an orderly shutdown in which previously submitted * tasks are executed, but no new tasks will be accepted...
Didn't we already determine that a Cleaner can't call shutdown() because that requires a strong reference it doesn't have?
It can't call shutdown() on a tracked pool object, but it could do something that acted equivalently as shutdown().
I think Doug already summed this up. The existing finalize() is pointless because when it could be called there is nothing left to be "cleaned up". There's no need for any use of Cleaner (even if it could do anything useful).
There's no need for finalize() or Cleaner in existing TPE as is, I agree. But there could be a thread pool that would self-shutdown when it is of no use any more (either using finalize() or Cleaner). For example, here is such pool: public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe; public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; } // implement and delegate all ExecutorService methods to tpe... } Regards, Peter
David
On 2/11/2017 7:26 PM, Peter Levart wrote:
On 11/02/2017 03:34 AM, David Holmes wrote:
On 2/11/2017 3:46 AM, Peter Levart wrote:
On 11/01/17 13:34, David Holmes wrote:
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.
Pool is actively in use when it is still reachable. Only in that case can new tasks be submitted. When it is not reachable any more, no new tasks can be submitted and it can be shutdown():
/** * Initiates an orderly shutdown in which previously submitted * tasks are executed, but no new tasks will be accepted...
Didn't we already determine that a Cleaner can't call shutdown() because that requires a strong reference it doesn't have?
It can't call shutdown() on a tracked pool object, but it could do something that acted equivalently as shutdown().
I think Doug already summed this up. The existing finalize() is pointless because when it could be called there is nothing left to be "cleaned up". There's no need for any use of Cleaner (even if it could do anything useful).
There's no need for finalize() or Cleaner in existing TPE as is, I agree. But there could be a thread pool that would self-shutdown when it is of no use any more (either using finalize() or Cleaner). For example, here is such pool:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... }
Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe. Though if you plan on abandoning a TPE such that you can't shutdown directly, then you may as well just configure it so all the threads terminate and it will "self-clean". Cheers, David
Regards, Peter
David
On 11/02/2017 01:47 PM, David Holmes wrote:
On 2/11/2017 7:26 PM, Peter Levart wrote:
On 11/02/2017 03:34 AM, David Holmes wrote:
On 2/11/2017 3:46 AM, Peter Levart wrote:
On 11/01/17 13:34, David Holmes wrote:
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.
Pool is actively in use when it is still reachable. Only in that case can new tasks be submitted. When it is not reachable any more, no new tasks can be submitted and it can be shutdown():
/** * Initiates an orderly shutdown in which previously submitted * tasks are executed, but no new tasks will be accepted...
Didn't we already determine that a Cleaner can't call shutdown() because that requires a strong reference it doesn't have?
It can't call shutdown() on a tracked pool object, but it could do something that acted equivalently as shutdown().
I think Doug already summed this up. The existing finalize() is pointless because when it could be called there is nothing left to be "cleaned up". There's no need for any use of Cleaner (even if it could do anything useful).
There's no need for finalize() or Cleaner in existing TPE as is, I agree. But there could be a thread pool that would self-shutdown when it is of no use any more (either using finalize() or Cleaner). For example, here is such pool:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... }
Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe.
Though if you plan on abandoning a TPE such that you can't shutdown directly, then you may as well just configure it so all the threads terminate and it will "self-clean".
True, but if I for some reason want to use a long keepAliveTime, the TPE will hang there unused for such amount of time after all executing tasks are already finished before idle threads finally terminate. Suppose I want to use TPE in a library where I don't have control over the life-cycle of the program and such library is used in an app deployed in app server. Each redeployment of such app will keep threads alive for more time than necessary. Frequent redeployment (such as when developing the app) might pile up more threads than available... Regards, Peter
Cheers, David
Regards, Peter
David
Hi, On 11/02/2017 01:47 PM, David Holmes wrote:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... }
Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe.
I see there already is the following method in Executors: public static ExecutorService newSingleThreadExecutor() { return new FinalizableDelegatedExecutorService (new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>())); } private static class FinalizableDelegatedExecutorService extends DelegatedExecutorService { FinalizableDelegatedExecutorService(ExecutorService executor) { super(executor); } @SuppressWarnings("deprecation") protected void finalize() { super.shutdown(); } } If the same FinalizableDelegatedExecutorService was used also for the following method: /** * Returns an object that delegates all defined {@link * ExecutorService} methods to the given executor, but not any * other methods that might otherwise be accessible using * casts. This provides a way to safely "freeze" configuration and * disallow tuning of a given concrete implementation. * @param executor the underlying implementation * @return an {@code ExecutorService} instance * @throws NullPointerException if executor null */ public static ExecutorService unconfigurableExecutorService(ExecutorService executor) { if (executor == null) throw new NullPointerException(); return new DelegatedExecutorService(executor); } ...we would get such ExecutorService out of the box. Regards, Peter
Peter, Executors.unconfigurableExecutorService was not modified on purpose because of the following case: http://cs.oswego.edu/pipermail/concurrency-interest/2006-March/002353.html Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Peter Levart <peter.levart@gmail.com> Sent: Thursday, November 2, 2017 10:23 AM To: David Holmes; Roger Riggs Cc: core-libs-dev Subject: Re: ThreadPoolExecutor and finalization Hi, On 11/02/2017 01:47 PM, David Holmes wrote:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... }
Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe.
I see there already is the following method in Executors: public static ExecutorService newSingleThreadExecutor() { return new FinalizableDelegatedExecutorService (new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>())); } private static class FinalizableDelegatedExecutorService extends DelegatedExecutorService { FinalizableDelegatedExecutorService(ExecutorService executor) { super(executor); } @SuppressWarnings("deprecation") protected void finalize() { super.shutdown(); } } If the same FinalizableDelegatedExecutorService was used also for the following method: /** * Returns an object that delegates all defined {@link * ExecutorService} methods to the given executor, but not any * other methods that might otherwise be accessible using * casts. This provides a way to safely "freeze" configuration and * disallow tuning of a given concrete implementation. * @param executor the underlying implementation * @return an {@code ExecutorService} instance * @throws NullPointerException if executor null */ public static ExecutorService unconfigurableExecutorService(ExecutorService executor) { if (executor == null) throw new NullPointerException(); return new DelegatedExecutorService(executor); } ...we would get such ExecutorService out of the box. Regards, Peter
Hi Jason, On 11/02/2017 05:13 PM, Jason Mehrens wrote:
Peter,
Executors.unconfigurableExecutorService was not modified on purpose because of the following case: http://cs.oswego.edu/pipermail/concurrency-interest/2006-March/002353.html
Jason
Oh, I see. The method is meant to provide a limited view over any ExecutorService and there may be multiple views over same instance. Regards, Peter
________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Peter Levart <peter.levart@gmail.com> Sent: Thursday, November 2, 2017 10:23 AM To: David Holmes; Roger Riggs Cc: core-libs-dev Subject: Re: ThreadPoolExecutor and finalization
Hi,
On 11/02/2017 01:47 PM, David Holmes wrote:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... } Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe. I see there already is the following method in Executors:
public static ExecutorService newSingleThreadExecutor() { return new FinalizableDelegatedExecutorService (new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>())); }
private static class FinalizableDelegatedExecutorService extends DelegatedExecutorService { FinalizableDelegatedExecutorService(ExecutorService executor) { super(executor); } @SuppressWarnings("deprecation") protected void finalize() { super.shutdown(); } }
If the same FinalizableDelegatedExecutorService was used also for the following method:
/** * Returns an object that delegates all defined {@link * ExecutorService} methods to the given executor, but not any * other methods that might otherwise be accessible using * casts. This provides a way to safely "freeze" configuration and * disallow tuning of a given concrete implementation. * @param executor the underlying implementation * @return an {@code ExecutorService} instance * @throws NullPointerException if executor null */ public static ExecutorService unconfigurableExecutorService(ExecutorService executor) { if (executor == null) throw new NullPointerException(); return new DelegatedExecutorService(executor); }
...we would get such ExecutorService out of the box.
Regards, Peter
Hi, I would think that the some part of the application or library would be responsible for the (lifecycle of the) executor that is being wrapped and it is not the responsibility of the 'view' to keep it alive. $.02, Roger On 11/2/2017 1:24 PM, Peter Levart wrote:
Hi Jason,
On 11/02/2017 05:13 PM, Jason Mehrens wrote:
Peter,
Executors.unconfigurableExecutorService was not modified on purpose because of the following case: http://cs.oswego.edu/pipermail/concurrency-interest/2006-March/002353.html
Jason
Oh, I see. The method is meant to provide a limited view over any ExecutorService and there may be multiple views over same instance.
Regards, Peter
________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Peter Levart <peter.levart@gmail.com> Sent: Thursday, November 2, 2017 10:23 AM To: David Holmes; Roger Riggs Cc: core-libs-dev Subject: Re: ThreadPoolExecutor and finalization
Hi,
On 11/02/2017 01:47 PM, David Holmes wrote:
public class CleanableExecutorService implements ExecutorService { private final ThreadPoolExecutor tpe;
public CleanableExecutorService(ThreadPoolExecutor tpe) { CleanerFactory.cleaner().register(this, tpe::shutdown); this.tpe = tpe; }
// implement and delegate all ExecutorService methods to tpe... } Ah I see - the old "extra level of indirection" solution. :) The Cleaner keeps the tpe strongly reachable, but as soon as the holder class becomes "unreachable" the Cleaner will shutdown the tpe. I see there already is the following method in Executors:
public static ExecutorService newSingleThreadExecutor() { return new FinalizableDelegatedExecutorService (new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>())); }
private static class FinalizableDelegatedExecutorService extends DelegatedExecutorService { FinalizableDelegatedExecutorService(ExecutorService executor) { super(executor); } @SuppressWarnings("deprecation") protected void finalize() { super.shutdown(); } }
If the same FinalizableDelegatedExecutorService was used also for the following method:
/** * Returns an object that delegates all defined {@link * ExecutorService} methods to the given executor, but not any * other methods that might otherwise be accessible using * casts. This provides a way to safely "freeze" configuration and * disallow tuning of a given concrete implementation. * @param executor the underlying implementation * @return an {@code ExecutorService} instance * @throws NullPointerException if executor null */ public static ExecutorService unconfigurableExecutorService(ExecutorService executor) { if (executor == null) throw new NullPointerException(); return new DelegatedExecutorService(executor); }
...we would get such ExecutorService out of the box.
Regards, Peter
Hi, So this thread died out a while ago with some alternatives discussed and no clear short term solution. I'd be happy if someone closer to the ThreadPoolExecutor would be able to take another look at the issues. For the time being, it is lower down on my priorities. Thanks, Roger [1] 8190324 ThreadPoolExecutor should not specify a dependency on finalization <https://bugs.openjdk.java.net/browse/JDK-8190324>
On 11/28/2017 01:11 PM, Roger Riggs wrote:
Hi,
So this thread died out a while ago with some alternatives discussed and no clear short term solution. I'd be happy if someone closer to the ThreadPoolExecutor would be able to take another look at the issues. For the time being, it is lower down on my priorities.
I still think the best move is to delete the method, and tweak the documentation. -Doug
+1 David On 29/11/2017 4:14 AM, Doug Lea wrote:
On 11/28/2017 01:11 PM, Roger Riggs wrote:
Hi,
So this thread died out a while ago with some alternatives discussed and no clear short term solution. I'd be happy if someone closer to the ThreadPoolExecutor would be able to take another look at the issues. For the time being, it is lower down on my priorities.
I still think the best move is to delete the method, and tweak the documentation.
-Doug
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 <david.holmes@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.
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
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%24j...
participants (9)
-
Andrej Golovnin
-
David Holmes
-
David Lloyd
-
Doug Lea
-
Jason Mehrens
-
Martin Buchholz
-
Peter Levart
-
Roger Riggs
-
Stuart Marks