On Fri, 5 May 2023 14:50:30 GMT, Afshin Zafari <d...@openjdk.org> wrote:
>> The `finalize()` method is removed from base classes/interfaces and are
>> replaced by a Cleaner callback..
>
> Afshin Zafari has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains four commits:
>
> - Merge branch 'master' into _8305083
> - 8305083: 8305083: Remove finalize() from
> test/hotspot/jtreg/vmTestbase/nsk/share/ and /jpda that are used in
> serviceability/dcmd/framework tests
> - 8305083: Remove finalize() from test/hotspot/jtreg/vmTestbase/nsk/share/
> and /jpda that are used in serviceability/dcmd/framework tests
> - 8305083: Remove finalize() from test/hotspot/jtreg/vmTestbase/nsk/share/
> and /jpda that are used in serviceability/dcmd/framework tests
This is a tricky issue to solve cleanly and minimally. If every class extended
FinalizableObject then all the implementation could go there, but we have to
try and split things across FinalizableObject and Finalizable because some
classes only implement the interface - this leads to some unfortunate design
choices.
I think a better design for the classes that can't extend FinalizableObject
would be for them to contain a FinalizableObject, the cleanup action for which
would cleanup the host object. That could allow the removal of the Finalizable
interface and simplify the general usage patterns. But that may be going too
far for this particular PR ... ?
test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 30:
> 28: * Finalizable interface allows <tt>Finalizer</tt> to perform
> finalization of an object.
> 29: * Each object that requires finalization at VM shutdown time should
> implement this
> 30: * interface and call the <tt>registerClenup</tt> to activate a
> <tt>Finalizer</tt> hook.
Typo: Clenup
test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 41:
> 39: * @see Finalizer
> 40: */
> 41: public void cleanup();
I see now that implementing `registerCleanup` as a default method forces
`cleanup` to become a public interface member. That is unfortunate and not what
we would generally do - but for this testing framework it may be okay.
test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 61:
> 59: */
> 60: default public void registerCleanup() {
> 61: // install finalizer to print errors summary at exit
This was moved from Log but isn't appropriate for the interface. No need to say
anything here.
test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 65:
> 63: finalizer.activate();
> 64:
> 65: // register the cleanup method to be called when this Log instance
> becomes unreachable.
Remove "Log" - actually again this is not needed. You can see what the method
does and it is already documented in the doc comment.
test/hotspot/jtreg/vmTestbase/nsk/share/FinalizableObject.java line 37:
> 35: /**
> 36: * All instances of this class, should implement their own cleanup
> method
> 37: * to clean appropriately the objects they used.
"instances" don't implement methods. Suggestion:
"Subclasses should override this method to provide the specific cleanup actions
that they need."
test/hotspot/jtreg/vmTestbase/nsk/share/MainWrapper.java line 50:
> 48: finalizableObject.registerCleanup();
> 49:
> 50:
Extra blank line not needed
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java line 27:
> 25:
> 26: import java.io.*;
> 27: import java.lang.ref.Cleaner;
Seems unnecessary
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java line 410:
> 408: *
> 409: * This is replacement of the deprecated finalize() and is called
> 410: * when this instance becomes unreachable.
I don't think this is needed.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java line 555:
> 553: *
> 554: * This is replacement of the finalize() method and is called when
> this
> 555: * instance becomes unreachable.
Again not needed.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 89:
> 87: this.log = binder.getLog();
> 88:
> 89: // As the alternative to finalize(), register the cleanup() method
No need to say "As an alternative to finalize()".
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 91:
> 89: // As the alternative to finalize(), register the cleanup() method
> 90: // to be called when this instance becomes unreachable.
> 91: Cleaner.create().register(this, () -> cleanup());
Why do we need to do this explicitly here? Why not call `registerCleaner`?
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/SocketIOPipe.java line 26:
> 24:
> 25: import java.io.IOException;
> 26: import java.lang.ref.Cleaner;
Not needed.
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13420#pullrequestreview-1415965638
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918144
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918976
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918246
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918278
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918472
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186918574
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186919232
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186919622
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186919786
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186919870
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186920211
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1186920560
