RFR: 8305083: Remove finalize() from test/hotspot/jtreg/vmTestbase/nsk/share/ and /jpda that are used in serviceability/dcmd/framework tests [v4]

[David Holmes]

On Fri, 5 May 2023 14:50:30 GMT, Afshin Zafari <duke at 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