RFR: 8304065: HttpServer.stop should terminate immediately if no exchanges are in progress [v4]

[Mark Sheppard]

On Thu, 22 May 2025 12:27:46 GMT, Mikhail Yankelevich <myankelevich at 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+dfuch at 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