RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v4]

[Alan Bateman]

On Thu, 23 Feb 2023 16:36:46 GMT, Martin Buchholz <martin at openjdk.org> wrote:

>> Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>> 
>>  - Merge
>>  - Improve SM scenario
>>  - Keep reference to Cleanable
>>  - Merge
>>  - Fix typo in comment, remove blank line
>>  - Replace older test
>>  - Initial commit
>
> src/java.base/share/classes/java/util/concurrent/Executors.java line 832:
> 
>> 830:     private static class AutoShutdownDelegatedExecutorService
>> 831:             extends DelegatedExecutorService {
>> 832:         private final Cleanable cleaner;
> 
> "cleaner" is a Cleanable, not a Cleaner. We should consistently use (as we do elsewhere in openjdk)
> 
> 
>  private final Cleanable cleanable;
> 
> 
> and fix the others to be consistent:
> 
> 
> ./sun/nio/ch/DatagramChannelImpl.java:115:    private final Cleanable cleaner;
> ./sun/nio/ch/NioSocketImpl.java:106:    private Cleanable cleaner;
> ./java/util/concurrent/Executors.java:832:        private final Cleanable cleaner;
> 
> 
> (BTW: I find Cleaners much harder to use than old finalize, and it looks like I'm not the only one!)

I'm rename the Cleaner in AutoShutdownDelegatedExecutorService but I'd prefer not rename fields in other areas, at least not part of this change.

> (BTW: I find Cleaners much harder to use than old finalize, and it looks like I'm not the only one!)

Finalization has many flaws and I think we could add that it was way too easy to add a finalize method without thinking too much about the implications. So yes, Cleaners do force you to separate out the resource and think about concurrency. It is an advanced feature, to be used very sparely, and we should continue to developers to close the resource explicitly.

> test/jdk/java/util/concurrent/Executors/AutoShutdown.java line 118:
> 
>> 116:      */
>> 117:     private ExecutorService getDelegate(ExecutorService executor) throws Exception {
>> 118:         Field eField = Class.forName("java.util.concurrent.Executors$DelegatedExecutorService")
> 
> I was under the impression that VarHandle/MethodHandle are generally preferred for writing white box tests (as I did myself in various WhiteBox.java tests).

I don't think so. If you search the tests for uses of "+open" then you'll see a variety of usages.

-------------

PR: https://git.openjdk.org/jdk/pull/12675