On Wed, 16 Jul 2025 11:30:02 GMT, Jaikiran Pai <j...@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
