On Mon, 26 May 2025 12:28:16 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. src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java line 100: > 98: // Add a new reply to the queue of unprocessed replies. > 99: try { > 100: 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2107600300
