On Thu, 27 Aug 2026 05:23:19 GMT, Valerie Peng <[email protected]> wrote:
>> This PR removes the finalize() methods from the SASL modules and replace >> them with Cleaner. Also did some minor refactoring so the sensitive info is >> only kept wherever necessary. >> >> I am still working on a regression test for the GssKrb5 classes, just want >> to get the source changes out while I work on it so there is more time for >> review. >> >> Thanks in advance for the review~ >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > removed the post-disposal check as the Cleanable.clean() guarantees the > action runs at most once. Changes requested by bchristi (Reviewer). test/jdk/com/sun/security/sasl/ClientCleaner.java line 42: > 40: > 41: public static void main(String[] args) throws Exception { > 42: String[] types = { "CramMD5", "Plaint" }; I advocate for at least having a basic test that confirms an object can become unreachable. Would it be practical to add that for `GssKrb5Client` and `GssKrb5Server`? test/jdk/com/sun/security/sasl/ClientCleaner.java line 93: > 91: type); > 92: } > 93: return new WeakReference<>(obj, queue); This may be moot, based on my other suggestions, but since we're waiting for the objects to be completely unreachable, would it be better to use `PhantomReference` here ? test/jdk/com/sun/security/sasl/ClientCleaner.java line 119: > 117: System.gc(); > 118: Thread.sleep(100); > 119: } A while back, we added `WhiteBox.waitForReferenceProcess()`. Along with `WhiteBox.fullGC()`, `WhiteBox` is a more consistent and reliable way to test GC cleanup. So, I recommend using that utility, if possible. (Another GC testing utility is `jdk.test.lib.util.ForceGC`.) A testing shorcut I use when just confirming that an object became unreachable is to add the object to a `WeakHashMap` and check for `whm.size() == 0` (or really, > 0). This is fewer LOC that setting up a `ReferenceQueue`, etc. For reference, the above techniques are used in this test: https://github.com/openjdk/jdk/blob/master/test/jdk/sun/security/smartcardio/TestCleaner.java#L98 ------------- PR Review: https://git.openjdk.org/jdk/pull/32551#pullrequestreview-5072881155 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3899751203 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3899756706 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3899788641
