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

[Daniel Fuchs]

On Tue, 20 May 2025 15:51:50 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.

Some early comments. I will review the test next.

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 252:

> 250:         selector.wakeup();
> 251:         long latest = System.nanoTime() + delay * 1000000000L;
> 252:         while (System.nanoTime() < latest) {

This is not correct as `latest` may overflow.

Suggestion:

        long start = System.nanoTime();
        while ((System.nanoTime() - start) / 1000_000L < delay ) {

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 `reqConnection.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 && reqConnection.isEmpty()) {


I suspect the other place where we set `finished = true` should also check that `reqConnection.isEmpty()` (and there we do need to check terminating).

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.

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

PR Review: https://git.openjdk.org/jdk/pull/25333#pullrequestreview-2854863433
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098395272
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098432081
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098444830