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