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

[Jaikiran Pai]

On Mon, 26 May 2025 15:57:17 GMT, Daniel Fuchs <dfuchs 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.
>
> 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