JDK-8195862 : ThreadPoolExecutor should not specify a dependency on finalization
  • Type: CSR
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Priority: P3
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 11
  • Submitted: 2018-01-22
  • Updated: 2018-02-09
  • Resolved: 2018-02-08
Related Reports
CSR :  
Description
Summary
-------

Remove the (useless) finalize method body from ThreadPoolExecutor

Problem
-------

The openjdk project is trying to get rid of finalizers, and ....

we finally decided that we should remove the finalize() method's body
from ThreadPoolExecutor, leaving an empty method with no throws spec for compatibility.

The conditions for it to run are the same as the conditions
under which running it has no visible effect except to
change runstate status, which in turn would not be visible,
except to a subclass that overrides finalize and checks,
and even then, there was not a guarantee about status
change, so we don't think this affects any actual usage.

Solution
--------

Just make the finalize method a no-op and make minor spec adjustments.

Specification
-------------

Just remove the finalize method body (it's already deprecated) and in addition:

```
- * <dt>Finalization</dt>
+ * <dt>Reclamation</dt>
  *
  * <dd>A pool that is no longer referenced in a program <em>AND</em>
- * has no remaining threads will be {@code shutdown} automatically. If
- * you would like to ensure that unreferenced pools are reclaimed even
- * if users forget to call {@link #shutdown}, then you must arrange
- * that unused threads eventually die, by setting appropriate
+ * has no remaining threads may be reclaimed (garbage collected)
+ * without being explicitly shutdown. You can configure a pool to
+ * allow all unused threads to eventually die by setting appropriate
  * keep-alive times, using a lower bound of zero core threads and/or
  * setting {@link #allowCoreThreadTimeOut(boolean)}.  </dd>
  *
@@ -361,7 +357,7 @@
      * time, but need not hit each state. The transitions are:
      *
      * RUNNING -> SHUTDOWN
-     *    On invocation of shutdown(), perhaps implicitly in finalize()
+     *    On invocation of shutdown()
      * (RUNNING or SHUTDOWN) -> STOP
      *    On invocation of shutdownNow()
      * SHUTDOWN -> TIDYING
```

```
+    // Override without "throws Throwable" for compatibility with subclasses
+    // whose finalize method invokes super.finalize() (as is recommended).
+    // Before JDK 11, finalize() had a non-empty method body.
+
     /**
-     * Invokes {@code shutdown} when this executor is no longer
-     * referenced and it has no threads.
-     *
-     * <p>This method is invoked with privileges that are restricted by
-     * the security context of the caller that invokes the constructor.
-     *
-     * @deprecated The {@code finalize} method has been deprecated.
-     *     Subclasses that override {@code finalize} in order to perform cleanup
-     *     should be modified to use alternative cleanup mechanisms and
-     *     to remove the overriding {@code finalize} method.
-     *     When overriding the {@code finalize} method, its implementation must explicitly
-     *     ensure that {@code super.finalize()} is invoked as described in {@link Object#finalize}.
-     *     See the specification for {@link Object#finalize()} for further
-     *     information about migration options.
+     * @implNote Previous versions of this class had a finalize method
+     * that shut down this executor, but in this version, finalize
+     * does nothing.
      */
     @Deprecated(since="9")
-    protected void finalize() {
-        SecurityManager sm = System.getSecurityManager();
-        if (sm == null || acc == null) {
-            shutdown();
-        } else {
-            PrivilegedAction<Void> pa = () -> { shutdown(); return null; };
-            AccessController.doPrivileged(pa, acc);
-        }
-    }
+    protected void finalize() {}
```
Comments
Description updated.
09-02-2018

Moving the request to approved contingent on the spec change in the Description being updated to what Martin has proposed; thanks.
08-02-2018

I can't imagine anyone objecting to this. Then again, my imagination has failed me about this CSR before...
08-02-2018

How about // Override without "throws Throwable" for compatibility with subclasses // whose finalize method invokes super.finalize() (as is recommended). // Before JDK 11, finalize() had a non-empty method body. /** * @implNote Previous versions of this class had a finalize method * that shut down this executor, but in this version, finalize * does nothing. */ @Deprecated(since="9") protected void finalize() {}
08-02-2018

Given the history of this class having a finalize method with a non-trivial method body, I don't think it is reasonable for the method to have no explicit doc in one release. If someone is doing archeology to see what changed in this finalizer across releases, if there is an explicit "This method does nothing" comment it is clearly an intentional change rather than a perhaps accidental deletion of javadoc.
08-02-2018

> There is a finalize method and it does nothing, why not say that. It's more useful to say nothing at all. > The generic inherited verbage does not provide any useful information about why there is a finalize method or what it does for this class. But ... that's why we now have --override-methods=summary , hiding any detail method javadoc generation! > The class javadoc note is fine but does not remove the need for doc on the finalize method itself. I disagree. We're spending too much effort on the doc niggles, and don't seem to be reaching consensus.
08-02-2018

There *is* a finalize method and it does nothing, why not say that. The generic inherited verbage does not provide any useful information about why there is a finalize method or what it does for this class. The class javadoc note is fine but does not remove the need for doc on the finalize method itself.
08-02-2018

Maybe it's just new-toy infatuation, but I don't want to give up the "no javadoc generated" by --override-methods=summary Instead, I propose we revive the "Finalization" section of the class javadoc as an implNote, and say there whatever we want to say about not doing any. How about """Previous versions of this class had a finalize method that shut down the executor, but this version does not."""
08-02-2018

I'm marking this request as pended until consensus is reached on how the finalize method should be documented.
07-02-2018

Martin, IIRC, the release note process is not externally documented (which would be reasonable to do). I've started the process by adding a label to the bug. Perhaps Roger can help with the remainder. In any case, given the history of this method over releases, I'd prefer some nominal javadoc appear for the method rather than just inheriting the default.
05-02-2018

Joe, deprecating a method for removal, when its superclass method is not, seems problematic because there's no @implSpec variant of @Deprecated. Taking advantage of --override-methods=summary removes most of the confusion (well ... at least for people reading the new spec). I don't know how to add a release note.
04-02-2018

Eventually I realized that we can use jdk10 javadoc's --override-methods=summary feature (which is already being used to build jdk javadocs), avoiding the need to have any javadoc here at all, and code like this: // Override without "throws Throwable" for compatibility with subclasses // whose finalize method invokes super.finalize() (as is recommended). // Before JDK 11, finalize() had a non-empty method body. // // javadoc intentionally elided, assuming --override-methods=summary @Deprecated(since="9") protected void finalize() {} which is what I think we want.
04-02-2018

Martin, at least one person on this thread has written devious tests from time to time ;-) Looking at evolving the spec over time, we generally want to avoid detailed exposition about why certain changes were made. (If nothing else, comments like "This new method in JDK 5..." age poorly and are are maintenance issue.) However, I think it would be avoided such concerns too much to not have any explicit spec here, especially as there is an override to provide a different exception contract, as described in the comments. It is true that the the nature of the specification here is better capture by an @implSpec tag rather than spec-spec working. I would find something like /** * A finalizer. * @implSpec This method has an empty body. * @deprecated ... */ preferable to the current proposed change. On a related note, I think it would in general be acceptable to deprecate a method like this for removal since it isn't *really* being removed per se since the inherited method from Object would be there. In other words, removing the overriden finalize in TPE would not be a binary incompatible change because of inheritance of the default method from Object. To err on the side of caution, I'd like to see a release note for this change.
03-02-2018

Mandy, We should not deprecate a method for removal if the superclass method is not deprecated for removal. (and I don't think Object.finalize should ever be removed, but that's a separate discussion)
03-02-2018

The only dependency possible is, as Martin indicates, directly calling finalize() from a subclass. If finalize() is only called as part of finalization then there is no impact at all - the whole starting point for this was that if finalize() were executing to shutdown the pool, then the pool must already be in a state where it would have shutdown. Hence the finalize() is completely redundant.
03-02-2018

Should the finalize method be deprecated for removal?
03-02-2018

Joe, "This finalizer has an empty body." is not in the spirit of Java - that's an implementation detail. The contract of ThreadPoolExecutor.finalize is (now) exactly the same as Object's method, except that it promises not to throw checked exceptions (which is another ancient mistake). We don't know of any ways for subclasses to detect this change but probably we haven't thought hard enough. Usually "any change is an incompatible change"..... Hmmmmmmmm.... yeah a subclass finalize method could call super.finalize and then somehow depend on the thread pool being shutdown, by checking that task submission fails. That's the sort of thing a devious test might do.
03-02-2018

I don't think inheriting the superclass javadoc for finalize is appropriate; I'd prefer something like "This finalizer has an empty body." or something equivalent. Is it possible for a subclass of ThreadPoolExecutor outside of the JDK to be relying on the behavior of ThreadPoolExecutor.finalize? For some other recent finalizer changes, the implementation was changed to be a no-op unless the subclass overrode the finalizer, in which case the old behavior at runtime was preserved.
03-02-2018

ok, that's fine.
30-01-2018

Roger, a javadoc with empty "main" body simply inherits superclass javadoc. make docs-jdk-api generates no warnings, and the generated javadoc includes Object's finalize spec as I expected.
30-01-2018

With the deletions, there is no first-line comment on the finalize method. It should have some comment to keep javadoc from complaining. "Finalizer is not used by ThreadPoolExecutor." or something simple.
30-01-2018

Reviewed.
30-01-2018

This seems fine to me. One nit: // Before jdk 11 -> // Before JDK 11
30-01-2018

Is there any actual incompatibility left now?
30-01-2018

Revised variant keeping a no-op finalize method with no throws spec.
30-01-2018

The no-op variant seems equally OK to me.
30-01-2018

Roger, I'm having trouble understanding the overall strategy for finalize deprecation. IMO finalize is a very long standing public API that simply cannot ever be removed. (but this is not the right forum to discuss that)
25-01-2018

Here's a test that demonstrates the compatibilty problem and would be fixed by reintroducing // Override without "throws Throwable" just for historic // compatibility for subclasses whose finalize method invokes // super.finalize(). @SuppressWarnings("deprecation") protected void finalize() {} public void testFinalizeMethodCallsSuperFinalize() { CustomTPE e = new CustomTPE(1, 1, LONG_DELAY_MS, MILLISECONDS, new LinkedBlockingQueue<Runnable>()) { /** A finalize method without "throws Throwable". */ protected void finalize() { super.finalize(); } }; try (PoolCleaner cleaner = cleaner(e)) { } }
25-01-2018

Retaining the finalize() method (and signature) with an empty method body and a no-op description, is the safest. It will not raise a source compatibility issue. And due to the optimization in hotspot the runtime overhead of tracking finalizable objects will be avoided. Later, when/if finalize can be completely removed the methods can be trivially removed. Though, in cases were there seem little likelihood of anyone subclassing, the method can be removed and be done with it.
23-01-2018

Deleted - I see the problem now.
23-01-2018

There's another general design question with ExecutorService, whether it should implement AutoCloseable that would call shutdown. I can see lots of possible uses for that, but my experience is biased by writing many tests that create and destroy ExecutorServices.
22-01-2018

I'm surprised I didn't know that Object.finalize specifies "throws Throwable". And TPE does not (arguably a bug). It's likely that there are subclasses of TPE in the wild that call super.finalize as recommended in the spec. So more source-compatible would be to retain a TPE.finalize method that retains the current signature but does nothing. Roger, what's our strategy for public classes with finalize methods that users may have subclassed? We can never compatibly remove them.
22-01-2018

The proposed change looks fine. Please update the compatibility checkboxes, add a brief rationale for the 'minimal' compatibly change, and note the minor source incompatibility. The removal of ThreadPoolExecutor.finalize() exposes subclasses to Object.finalize() that throws Throwable. (You may need to 'Edit' the issue to see the compatibility fields).
22-01-2018

Inadvertently committed as ThreadPoolExecutor.java changes in http://hg.openjdk.java.net/jdk/jdk/rev/946e34c2dec9
22-01-2018