On Tue, 20 May 2025 16:58:02 GMT, Daniel Fuchs <dfu...@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
