On Mon, 26 May 2025 16:52:56 GMT, Daniel Fuchs <dfu...@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
