On Mon, 26 May 2025 15:57:17 GMT, Daniel Fuchs <dfu...@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. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2107620182
