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

[Shaojin Wen]

On Wed, 16 Jul 2025 11:30:02 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - merge latest from master branch
>  - merge latest from master branch
>  - add test
>  - 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed

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

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

Suggestion:

            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.

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

> 145:         }
> 146:         if (result == CANCELLED_MARKER) {
> 147:             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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2255863225
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2255865709