On Thu, 23 Feb 2023 16:36:46 GMT, Martin Buchholz <mar...@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
