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.
test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 128:
> 126:
> 127: // Time the shutdown sequence
> 128: final Duration delayDuration = Duration.ofSeconds(2);
We should use a greater margin here since we're not expecting to wait for that
duration, and probably use `Utils.adjustTimeout()` too. I'd suggest something
like:
Suggestion:
final Duration delayDuration =
Duration.ofSeconds(Utils.adjustTimeout(5));
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));
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));
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?
test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 228:
> 226: .uri(URI.create("http://";
> 227: +
> server.getAddress().getAddress().getHostAddress()
> 228: + ":" + server.getAddress().getPort() +
> "/"))
We should use `URIBuilder` here to take care of IPv4 vs IPv6 etc...
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
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098475385
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098478903
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098481789
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098485308
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098488908
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2098489836
