On Fri, 23 May 2025 08:48:32 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
> If I have read the original code correctly there is a flaw in it, that is the
> startExchange is never called. Thus the code in handleEvent won’t executed as
> anticipated
>
> 391 int exchanges = endExchange(); 392 if (terminating && exchanges == 0) {
> 393 finished = true; 394 }
>
> Exchanges will always be negative. This is inherited in the new update
>
> WRT current PR, I don't think it is good that the state of the server is
> encapsulated in the state of the CountDownLatch. The use of the
> CountDownLatch to synchronise the stop and the Dispatcher shutdown is fine,
> but having the server state encapsulated in the CountDownLatch value is a bit
> oblique or obfuscating. I think the original volatile boolean variable is a
> clearer semantics
>
> I would make stop synchronised to have only one thread executing at time.
> First check on entry then becomes test on the server's current state. This
> means that this state management needs some attention.
>
> The addition of Stop event is a neat idea, but draws attention to the
> semantics of the stop. What is the desired semantics of stop, to not accept
> any new connections, i.e. stop the listener immediately, not accept new
> requests on existing connections. To allow existing Exchanges to complete and
> then shutdown all extant connections.
startExchange() is called in the constructor for ExchangeImpl, which is not
changing in this PR.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/25333#issuecomment-2912550272
