8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #25242

bchristi-git · 2025-05-14T23:51:36Z

Please review this change to replace the finalizer in AbstractLdapNamingEnumeration with Cleaner.

(The first PR for this fix started some substantial discussions, leading to, among other things, the 8314480 java.lang.ref memory ordering spec update.)

In standard fashion, pieces of state required for cleanup (LdapCtx homeCtx, LdapResult res, and LdapClient enumClnt) are moved into a Context object. From there, the change is fairly mechanical.

Details of note:

Some operations need to change the state values (the update() method is probably the most interesting). Use of reachabilityFence() ensures memory visibility on the Cleaner thread (per the aforementioned spec update).
Subclasses need to access homeCtx; I added a homeCtx() method to read homeCtx from the superclass's state.

The test case is based on a copy of com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java. It confirms that the use of Cleaner does not keep an LdapSearchEnumeration object reachable. The other AbstractLdapNamingEnumeration subclasses (LdapNamingEnumeration and LdapBindingEnumeration) can be expected to behave the same.

Thanks.

Edit: (Re)viewers: due to there being a lot of indentation changes, you might consider enabling the "Hide whitespace" option on the "Files changed" tab. To my eye, it gives a better view of the changes.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner (Sub-task - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25242/head:pull/25242
$ git checkout pull/25242

Update a local copy of the PR:
$ git checkout pull/25242
$ git pull https://git.openjdk.org/jdk.git pull/25242/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25242

View PR using the GUI difftool:
$ git pr show -t 25242

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25242.diff

Using Webrev

Link to Webrev Comment

… Cleaner

…umeration for consistencty ; new instance vars can be final

…er in the code

…ome unneeded comments

bridgekeeper · 2025-05-14T23:52:22Z

👋 Welcome back bchristi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-05-14T23:52:45Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-05-14T23:53:25Z

@bchristi-git The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-05-14T23:56:04Z

Webrevs

01: Full - Incremental (c3a94433)
00: Full (e36cf5db)

test/jdk/com/sun/jndi/ldap/CleanerTests/LdapEnumeration.ldap

viktorklang-ora · 2025-06-10T09:11:21Z

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java

-            this.homeCtx = homeCtx;
-            homeCtx.incEnumCount();
-            enumClnt = homeCtx.clnt; // remember
+            this.enumCtx.homeCtx.incEnumCount();


@bchristi-git Would it make sense to increment this in the EnumCtx ctor instead?

viktorklang-ora · 2025-06-10T09:20:36Z

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java

-    protected final void finalize() {
-        cleanup();


viktorklang-ora · 2025-06-10T09:22:52Z

src/java.naming/share/classes/com/sun/jndi/ldap/LdapSearchEnumeration.java

+        // No reachabilityFence here for now.
+        // super() call (aka AbstrctLdapNamingEnumeration ctor) keeps 'this'
+        // reachable until end of Cleaner registration. Code after that
+        // clearly does not touch cleanable state.


@bchristi-git I fear this could be a brittle assumption. Would an extra reachabilityFence here hurt?

dfuch

Generally looks good. I haven't verified whether all methods that need a reachabilityFence have it. This will require further analysis (from me).
As Alan noted it would be really good if we could use try-with-resource, but I am not expert enough in LDAP search implementation to figure out whether that's feasible.

Your proposed fix preseves the status quo - where a finalizer has been replaced by a cleaner - and asserting that the two are equivalent seems more trackable for now.

dfuch · 2025-06-10T18:08:58Z

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java

-            errEx = ne.errEx;
+        try {
+            // Cleanup previous context first
+            getHomeCtx().decEnumCount();


What is the reason for calling getHomeCtx() here when we use enumCtxt.homeCtx a few lines below?

bridgekeeper · 2025-07-08T23:24:27Z

@bchristi-git This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2025-08-06T05:34:54Z

@bchristi-git This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bchristi-git added 29 commits May 12, 2025 14:37

Replace AbstractLdapNamingEnumeration finalizer with Cleaner

e0ffa86

Create copy of AddNewEntry test to test AbstractLdapNamingEnumeration…

a2a3d88

… Cleaner

Rename test to LdapEnumeration

0c4ea2a

Test changes to test new cleaner code

b3bb11c

fix whitespace

5806b1d

Rename inner class to EnumCtx ; use homeCtx() in AbstractLdapNamingEn…

1813008

…umeration for consistencty ; new instance vars can be final

Fix the merge fix

c70b8cd

merge

868cd6e

merge

df18ed4

Check that cleanup was performed correctly

4ed7818

Update .ldap capture file

5ef3bc0

test comment udpate

f6fa7b7

merge

731bf51

Update test files - add copyright, etc

dafb973

Restore comments in ldap capture file

3f70a02

Restore EnumCtx to being an inner class, to keep all the state togeth…

6a1e9a4

…er in the code

reference emunCtx and homeCtx consistently in constructor

a66bff9

merge

2dfe6fb

Update ForceGC usage

6b7b27c

simplify some comments

489bf61

remove fullFence() calls, fix reachabilityFence() arguments, remove s…

b011ced

…ome unneeded comments

update EnumCtx comment

8c272d3

fix hasMoreReferrals() merge error

6ee23e1

more hasMoreReferrals() merge error fixing

b6848e7

remove unneeded fullFence()s, fix copyright years, update some comments

c52f9ad

improve test to give cleaning action a chance to run

f8a8eca

update test to use WhiteBox, and use volatile read for enumCount

5d2cbd0

simplify test

5e0c304

small comment update

e36cf5d

openjdk bot added the rfr Pull request is ready for review label May 14, 2025

openjdk bot added the core-libs core-libs-dev@openjdk.org label May 14, 2025

ky0n reviewed May 15, 2025

View reviewed changes

test/jdk/com/sun/jndi/ldap/CleanerTests/LdapEnumeration.ldap Outdated Show resolved Hide resolved

update copyright

c3a9443

viktorklang-ora reviewed Jun 10, 2025

View reviewed changes

dfuch reviewed Jun 10, 2025

View reviewed changes

bridgekeeper bot closed this Aug 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #25242

8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #25242

Uh oh!

bchristi-git commented May 14, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented May 14, 2025

Uh oh!

openjdk bot commented May 14, 2025

Uh oh!

openjdk bot commented May 14, 2025

Uh oh!

mlbridge bot commented May 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

viktorklang-ora Jun 10, 2025

Uh oh!

viktorklang-ora Jun 10, 2025

Uh oh!

viktorklang-ora Jun 10, 2025

Uh oh!

dfuch left a comment

Uh oh!

dfuch Jun 10, 2025

Uh oh!

bridgekeeper bot commented Jul 8, 2025

Uh oh!

bridgekeeper bot commented Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #25242

8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #25242

Uh oh!

Conversation

bchristi-git commented May 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented May 14, 2025

Uh oh!

openjdk bot commented May 14, 2025

Uh oh!

openjdk bot commented May 14, 2025

Uh oh!

mlbridge bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

viktorklang-ora Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

dfuch Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jul 8, 2025

Uh oh!

bridgekeeper bot commented Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

bchristi-git commented May 14, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented May 14, 2025 •

edited

Loading