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

[Mikhail Yankelevich]

On Tue, 20 May 2025 16:58:02 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Mikhail Yankelevich has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   addressing comments
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 409:
> 
>> 407:             // termination is in progress and exchange count is 0
>> 408:             if (r instanceof StopRequestedEvent) {
>> 409:                 logger.log(Level.TRACE, "Stop event requested");
> 
> Suggestion:
> 
>                 logger.log(Level.TRACE, "Handling Stop Requested Event");

Done in the next commit

> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 411:
> 
>> 409:                 logger.log(Level.TRACE, "Stop event requested");
>> 410: 
>> 411:                 if (terminating && getExchangeCount() == 0) {
> 
> It looks like we may have to check `reqConnections.isEmpty()` here too for the case where the `ServerImpl` is configured with an executor, as it looks like the `exchangeCount` will be incremented in the executor. It would be interesting to try & craft a test with that scenario - possibly by configuring the server with an executor that sleeps some before calling `run()`, thus giving the oportunity of the `StopRequestedEvent` to be handled before the `ExchangeImpl` is created.
> 
> Also if we reach here we are terminating - so we could simply assert that.
> 
> Suggestion:
> 
>                 boolean terminating = this.terminating;
>                 assert terminating;
>                 if (getExchangeCount() == 0 && reqConnections.isEmpty()) {
> 
> 
> I suspect the other place where we set `finished = true` should also check that `reqConnections.isEmpty()` (and there we do need to check terminating).

Done in the next commit

> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 413:
> 
>> 411:                 if (terminating && getExchangeCount() == 0) {
>> 412:                     finished = true;
>> 413:                 }
> 
> Suggestion:
> 
>                 } else {
>                     logger.log(Level.TRACE, "Some requests are still pending");
>                 }

Done in the next commit

> src/jdk.httpserver/share/classes/sun/net/httpserver/StopRequestedEvent.java line 30:
> 
>> 28: /**
>> 29:  * Stopping event for the http server.
>> 30:  * Does not contain ay information about a connection.
> 
> Suggestion:
> 
>  * The event applies to the whole server and is not tied to any particular exchange.

Done in the next commit

> test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 160:
> 
>> 158:         // Complete the exchange 10 second into the future.
>> 159:         // Runs in parallel, so won't block the server stop
>> 160:         final Duration exchangeDuration = Duration.ofSeconds(10);
> 
> Suggestion:
> 
>         final Duration exchangeDuration = Duration.ofSeconds(Utils.adjustTimeout(5));

Done in the next commit

> test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 190:
> 
>> 188:     public void noActiveExchanges() {
>> 189:         // With no active exchanges, shutdown should complete immediately
>> 190:         final Duration delayDuration = Duration.ofSeconds(1);
> 
> Suggestion:
> 
>         final Duration delayDuration = Duration.ofSeconds(Utils.adjustTimeout(5));

Done in the next commit

> test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 207:
> 
>> 205:         timeShutdown(delayDuration);
>> 206:         log("Shutting down the server the second time");
>> 207:         timeShutdown(delayDuration);
> 
> Should we check the time it took the server to shutdown here too?

This test just validates, that there is no error thrown. The other tests cover the timing scenarios

> test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 238:
> 
>> 236:                     // count the latch down to allow the handler to complete
>> 237:                     // and the server's dispatcher thread to proceed; The handler
>> 238:                     // is called withing the dispatcher thread since we haven't
> 
> Suggestion:
> 
>                     // is called within the dispatcher thread since we haven't

Done in the next commit

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100728907
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100724961
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100730146
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100729880
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100730296
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100730441
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100732198
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2100732503