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

[Jaikiran Pai]

On Mon, 26 May 2025 16:52:56 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> 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.
>
>> 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. 
> 
> I am not seeing that - close() is called by Connection.cleanup() which seems to be callable asynchronously.

I forgot to reply here, but yes Daniel is right that the `close()` method can be called concurrently. With the current proposed change, it should be OK to have close() be invoked concurrently. The `close()/cancel()` invocation will place the respective marker in the queue and will also mark the close/cancel flag. We intentionally place the close/cancel markers in the queue so that  the `getReplyBer()` will find that marker in the right order, when it is next invoked or if it is currently blocked waiting for the next message.

Given the current expected semantics of `getReplyBer()`, we skip adding any new replies after the close/cancel markers have been placed in the queue. But that's on a best effort basis. Due to a race, if we do add replies to the queue after the close/cancel has been invoked, then it's OK because the `getReplyBer()` upon noticing the close/cancel markers first, will not process the subsequent replies in the queue. Thus it retains the current behaviour of not processing any replies after close/cancel has been noticed.

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

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