8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed #25449

jaikiran · 2025-05-26T12:28:16Z

Can I please get a review of this change which proposes to address the issue noted in https://bugs.openjdk.org/browse/JDK-8357708?

As noted in the issue, the current code in com.sun.jndi.ldap.Connection.readReply() is susceptible to throwing a ServiceUnavailableException even when the LDAP replies have already been received and queued for processing. The JBS issue has additional details about how that can happen.

The commit in this PR simplifies the code in com.sun.jndi.ldap.LdapRequest to make sure it always gives out the replies that have been queued when the LdapRequest.getReplyBer() gets invoked. One of those queued values could be markers for a cancelled or closed request. In that case, the getReplyBer(), like previously, continues to throw the right exception. With this change, the call to replies.take() or replies.poll() (with an infinite timeout) is no longer expected to hang forever, if the Connection is closed (or the request cancelled). This then allows us to remove the connection closure (sock == null) check in Connection.readReply().

A new jtreg test has been introduced to reproduce this issue and verify the fix. The test reproduces this issue consistently when the source fix isn't present. With the fix present, even after several thousand runs of this test, the issue no longer reproduces.

tier1, tier2 and tier3 tests continue to pass with this change. I've marked the fix version of this issue for 26 and I don't plan to push this for 25.

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-8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed (Bug - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer) Review applies to ca8c19a8
Aleksei Efimov (@AlekseiEfimov - Reviewer)

Contributors

Aleksei Efimov <aefimov@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25449

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…Connection is subsequently closed

bridgekeeper · 2025-05-26T12:29:05Z

👋 Welcome back jpai! 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-26T12:29:40Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed

Co-authored-by: Aleksei Efimov <aefimov@openjdk.org>
Reviewed-by: aefimov, dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the master branch:

44aad07: 8328874: Class::forName0 should validate the class name length early
10fea86: 8367371: Remove @requires vm.opt.UseLargePages from InternSharedString.java test
02d7281: 8367483: C2 crash in PhaseValues::type: assert(t != nullptr) failed: must set before get - missing notification for CastX2P(SubL(x, y))
... and 7 more: https://git.openjdk.org/jdk/compare/5abd18426d64f878ca45f9d36ca270be17a7760f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-05-26T12:30:13Z

@jaikiran 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-26T12:33:31Z

Webrevs

dfuch · 2025-05-26T15:57:17Z

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

-            lock.unlock();
+        // Add a new reply to the queue of unprocessed replies.
+        try {
+            replies.put(ber);


So theoretically this could get into the queue after one of the two markers has already been put in the queue, since we no longer use the lock. The question is: is this a problem? I'd be tempted to say no - except that getReplyBer() will take the markers out of the queue.

Hello Daniel,

I'd be tempted to say no - except that getReplyBer() will take the markers out of the queue.

That's correct - the change intentionally removes the lock and also lets the close/cancel markers land into the queue so that if the getReplyBer() is already blocked in a take() or poll() call, then it will be unblocked and if it isn't yet blocked on those calls then a subsequent call will find these markers (which are distinct for close and cancel).

Adding these (distinct) markers will also allow for an ordered identifiable content in the queue - some replies followed by close/cancel marker or a close/cancel marker followed by replies.

Additionally, the addRequest(), close() and cancel() methods of this LdapRequet get called by a single thread managed by the Connection class, so there isn't expected to be concurrent calls across these methods. So, I think, removing the lock and letting the (distinct) markers end up in the queue makes this code in the LdapRequest simpler.

The getReplyBer() the implementation polls the ordered queue, so it find all replies that have arrived before the cancel/close marker is encountered. Once it encounters those markers it can then throw the relevant exception as per its current specified behaviour.

Additionally, the addRequest(), close() and cancel() methods of this LdapRequet get called by a single thread managed by the Connection class, so there isn't expected to be concurrent calls across these methods.

I am not seeing that - close() is called by Connection.cleanup() which seems to be callable asynchronously.

I forgot to reply here, but yes Daniel is right that the close() method can be called concurrently. With the current proposed change, it should be OK to have close() be invoked concurrently. The close()/cancel() invocation will place the respective marker in the queue and will also mark the close/cancel flag. We intentionally place the close/cancel markers in the queue so that the getReplyBer() will find that marker in the right order, when it is next invoked or if it is currently blocked waiting for the next message.

Given the current expected semantics of getReplyBer(), we skip adding any new replies after the close/cancel markers have been placed in the queue. But that's on a best effort basis. Due to a race, if we do add replies to the queue after the close/cancel has been invoked, then it's OK because the getReplyBer() upon noticing the close/cancel markers first, will not process the subsequent replies in the queue. Thus it retains the current behaviour of not processing any replies after close/cancel has been noticed.

bridgekeeper · 2025-07-04T09:15:43Z

@jaikiran 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!

AlekseiEfimov · 2025-07-16T10:01:31Z

/keepalive

openjdk · 2025-07-16T10:02:36Z

@AlekseiEfimov The pull request is being re-evaluated and the inactivity timeout has been reset.

wenshao · 2025-08-06T04:57:12Z

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

+                : replies.take();
        // poll from 'replies' blocking queue ended-up with timeout
        if (result == null) {
            throw new IOException(String.format(TIMEOUT_MSG_FMT, millis));


Suggested change

throw new IOException(String.format(TIMEOUT_MSG_FMT, millis));

throw new IOException("LDAP response read timed out, timeout used: %d ms.".formatted(millis));

TIMEOUT_MSG_FMT is only used once here, and defining a constant would make the code less readable.

wenshao · 2025-08-06T04:59:33Z

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

-        // Unexpected EOF can be caused by connection closure or cancellation
-        if (result == EOF) {
+        if (result == CANCELLED_MARKER) {
+            throw new CommunicationException("Request: " + msgId +


The current LdapRequest class should use a unified style to construct exception information, either using string concatenation or String.format. A class should not mix the two styles.

Hello Shaojin, these are pre-existing messages. I have however updated the PR to use a uniform style.

wenshao · 2025-08-06T05:03:16Z

test/jdk/javax/naming/ldap/LdapClientConnTest.java

+        final int numTasks = 10;
+        try (final ExecutorService executor = Executors.newCachedThreadPool()) {
+            for (int i = 1; i <= numTasks; i++) {
+                final String taskName = "task-" + i;


Suggested change

final String taskName = "task-" + i;

String taskName = "task-" + i;

Variables used only in the current block do not need to be final

This a more a personal preference and since this is a new test file, I let it stay in this form. If there's a strong preference to remove it, I'll do so.

wenshao · 2025-08-06T05:15:46Z

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

                ber.parseSeq(null);
                ber.parseInt();
-                completed = (ber.peekByte() == LdapClient.LDAP_REP_RESULT);
+                isLdapResResult = (ber.peekByte() == LdapClient.LDAP_REP_RESULT);


Other boolean type variables in the LdapRequest class do not have the is prefix. The local variable isLdapResResult here should also use the same style. We should not use two styles in one class.

Calling it ldapResResult instead of isLdapResResult isn't appropriate here, so I've let this stay in its current form.

bridgekeeper · 2025-09-03T05:22:13Z

@jaikiran 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!

dfuch · 2025-09-10T15:18:25Z

test/jdk/javax/naming/ldap/LdapClientConnTest.java

+ * @modules java.naming/com.sun.jndi.ldap
+ * @library /test/lib
+ * @build jdk.test.lib.net.URIBuilder
+ * @run junit LdapClientConnTest


Since this test creates a daemon thread and does not try to join the thread at the end it might be more prudent to run it in /othervm mode?

Hello Daniel, good catch. It took me a while to understand this - leaving around arbitrary test specific threads in an agent VM isn't wise. You are right, making it othervm would be better. I've updated the PR to do so.

dfuch

LGTM

AlekseiEfimov · 2025-09-12T14:43:57Z

Hello Jaikiran,

The fix and its logic looks correct to me.

I have a couple of suggestions regarding the new test:

The test can be modified to remove the dependency on the internal LdapCtx class. Something like:

@@ -43,11 +43,14 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 
-import com.sun.jndi.ldap.LdapCtx;
 import jdk.test.lib.net.URIBuilder;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+
+import javax.naming.Context;
+import javax.naming.InitialContext;
+
 import static java.nio.charset.StandardCharsets.US_ASCII;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -56,7 +59,6 @@
  * @bug 8357708
  * @summary verify that com.sun.jndi.ldap.Connection does not ignore the LDAP replies
  *          that were received before the Connection was closed.
- * @modules java.naming/com.sun.jndi.ldap
  * @library /test/lib
  * @build jdk.test.lib.net.URIBuilder
  * @run junit/othervm LdapClientConnTest
@@ -381,17 +383,22 @@
 
         @Override
         public Void call() throws Exception {
-            LdapCtx ldapCtx = null;
+            Context ldapCtx = null;
             try {
                 final InetSocketAddress serverAddr = server.getAddress();
                 final Hashtable<String, String> envProps = new Hashtable<>();
                 // explicitly set LDAP version to 3 to prevent LDAP BIND requests
                 // during LdapCtx instantiation
                 envProps.put("java.naming.ldap.version", "3");
-                ldapCtx = new LdapCtx("",
-                        serverAddr.getAddress().getHostAddress(),
-                        serverAddr.getPort(),
-                        envProps, false);
+                envProps.put(Context.INITIAL_CONTEXT_FACTORY,
+                        "com.sun.jndi.ldap.LdapCtxFactory");
+                String providerUrl = URIBuilder.newBuilder()
+                        .scheme("ldap")
+                        .host(serverAddr.getAddress())
+                        .port(serverAddr.getPort())
+                        .build().toString();
+                envProps.put(Context.PROVIDER_URL, providerUrl);
+                ldapCtx = new InitialContext(envProps);
                 final String name = SEARCH_REQ_DN_PREFIX + taskName + SEARCH_REQ_DN_SUFFIX;
                 // trigger the LDAP SEARCH requests through the lookup call. we are not
                 // interested in the returned value and are merely interested in a normal

Maybe move the new test to the test/jdk/com/sun/jndi/ldap folder where other tests for internal LDAP classes live.
I like the dynamic approach on generating and parsing the LDAP messages. For future tests in JNDI/LDAP area such approach could be beneficial. Therefore, a suggestion - move private static final byte BER_TYPE_* constants to the test/jdk/com/sun/jndi/ldap/lib/LDAPTestUtils.java.

…un.jndi.ldap.LdapCtx

jaikiran · 2025-09-12T16:05:37Z

/contributor @AlekseiEfimov

openjdk · 2025-09-12T16:07:43Z

@jaikiran Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

jaikiran · 2025-09-12T16:13:30Z

Hello Aleksei, I have updated the PR to implement your suggestion 1 and 2. As for the other suggestion of moving these constants to the LDAPTestUtils test library class, I think that's a good idea too. However, I gave that a try and that isn't straightforward. Once I move them to that class, jtreg then requires me to add a @build and a @library instruction to bring in the test.LDAPServer class which the LDAPTestUtils uses (but not this new test). That wouldn't be too bad and I did add those, but then LDAPTestUtils has an import of an internal class:

import com.sun.jndi.ldap.LdapURL

so that then forces me to reintroduce the:

@modules java.naming/com.sun.jndi.ldap

in this new test, which I think defeats the entire cleanup. So I decided to leave those constants this in this test class for now and reconsider that move as a future work. Would that be OK?

jaikiran · 2025-09-12T16:14:26Z

/contributor add @AlekseiEfimov

openjdk · 2025-09-12T16:14:39Z

@jaikiran
Contributor Aleksei Efimov <aefimov@openjdk.org> successfully added.

AlekseiEfimov · 2025-09-12T16:19:22Z

Hello Aleksei, I have updated the PR to implement your suggestion 1 and 2. As for the other suggestion of moving these constants to the LDAPTestUtils test library class, I think that's a good idea too. However, I gave that a try and that isn't straightforward. Once I move them to that class, jtreg then requires me to add a @build and a @library instruction to bring in the test.LDAPServer class which the LDAPTestUtils uses (but not this new test). That wouldn't be too bad and I did add those, but then LDAPTestUtils has an import of an internal class:
import com.sun.jndi.ldap.LdapURL
so that then forces me to reintroduce the:
@modules java.naming/com.sun.jndi.ldap
in this new test, which I think defeats the entire cleanup. So I decided to leave those constants this in this test class for now and reconsider that move as a future work. Would that be OK?

Oh, it is complicated, thank you for trying to move the constants.
I agree that it defeats the entire cleanup initiative and doesn't worth the effort.
It is OK to leave the constants in the test class.

AlekseiEfimov

Looks good to me!

jaikiran · 2025-09-12T16:29:28Z

Thank you everyone for the reviews. I've triggered a CI run with these latest changes and when that completes I'll go ahead and integrate this.

jaikiran · 2025-09-13T02:02:18Z

/integrate

openjdk · 2025-09-13T02:03:39Z

Going to push as commit e2eaa2e.
Since your change was applied there have been 17 commits pushed to the master branch:

3eb3e0d: 8366833: Poly1305 does not always correctly update position for array-backed ByteBuffers after processMultipleBlocks
35dabb1: 8244336: Restrict algorithms at JCE layer
84aa295: 8367368: Add message for verify_legal_class_modifiers for inner classes
... and 14 more: https://git.openjdk.org/jdk/compare/5abd18426d64f878ca45f9d36ca270be17a7760f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-13T02:03:48Z

@jaikiran Pushed as commit e2eaa2e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jaikiran added 2 commits May 26, 2025 11:45

8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if …

a1e1c0c

…Connection is subsequently closed

add test

908e4b6

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

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

dfuch reviewed May 26, 2025

View reviewed changes

merge latest from master branch

5a6012e

merge latest from master branch

545c41a

wenshao reviewed Aug 6, 2025

View reviewed changes

jaikiran added 2 commits September 9, 2025 12:50

merge latest from master branch

7e3bfe9

remove format() call in exception message creation

5d6c2fd

dfuch reviewed Sep 10, 2025

View reviewed changes

jaikiran added 2 commits September 12, 2025 11:54

merge latest from master branch

87b68f1

Daniel's review - make the test othervm

ca8c19a

dfuch approved these changes Sep 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2025

jaikiran added 2 commits September 12, 2025 21:08

Aleksei's review - use public/standard APIs instead of internal com.s…

b4b1e15

…un.jndi.ldap.LdapCtx

move the new test under test/jdk/com/sun/jndi/ldap

c5a222a

openjdk bot removed the ready Pull request is ready to be integrated label Sep 12, 2025

AlekseiEfimov approved these changes Sep 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2025

openjdk bot closed this Sep 13, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 13, 2025

jaikiran deleted the 8357708 branch September 13, 2025 02:07

	throw new IOException(String.format(TIMEOUT_MSG_FMT, millis));
	throw new IOException("LDAP response read timed out, timeout used: %d ms.".formatted(millis));

	final String taskName = "task-" + i;
	String taskName = "task-" + i;

8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed #25449

8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed #25449

Uh oh!

Conversation

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

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented May 26, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented May 26, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jul 4, 2025

Uh oh!

AlekseiEfimov commented Jul 16, 2025

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseiEfimov commented Sep 12, 2025

Uh oh!

jaikiran commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

jaikiran commented Sep 12, 2025

Uh oh!

jaikiran commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

AlekseiEfimov commented Sep 12, 2025

Uh oh!

AlekseiEfimov left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Sep 12, 2025

Uh oh!

jaikiran commented Sep 13, 2025

jaikiran commented May 26, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 26, 2025 •

edited

Loading

mlbridge bot commented May 26, 2025 •

edited

Loading