On Thu, 22 May 2025 12:27:46 GMT, Mikhail Yankelevich
<myankelev...@openjdk.org> wrote:
>> HttpServer::stop will terminate the server immidiately after all exhcnages
>> are complete.
>> If the exchanges take longer then the specified delay it will terminate
>> straight after the delay, the same as the previous behaviour.
>>
>> Used to wait until the delay is complete at all times, regardless of the
>> number of active exchanges.
>>
>> Tests based on @eirbjo work, so adding Eirik as a contributor.
>
> Mikhail Yankelevich has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Suggestions from cr
>
> Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com>
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.
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 227:
> 225: }
> 226:
> 227: public final boolean isFinishing() {
encapsulating the server state in a CountDoenLatch is obfuscated semantics
-------------
PR Comment: https://git.openjdk.org/jdk/pull/25333#issuecomment-2903739980
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2104135665
