On Wed, 21 May 2025 16:50:28 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.
>
> Mikhail Yankelevich has updated the pull request incrementally with one
> additional commit since the last revision:
>
> cleanup
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 228:
> 226:
> 227: public final boolean isFinishing() {
> 228: return finishedLatch.getCount()==0;
Suggestion:
return finishedLatch.getCount() == 0;
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 238:
> 236: * but still stops as soon as maximum delay is reached.
> 237: *
> 238: * @param delay maximum delay in stopping the server
Suggestion:
* This ensures that the server is stopped immediately after all exchanges
are complete. HttpConnections will be forcefully closed if active exchanges do
not
* complete within the imparted delay.
*
* @param delay maximum delay to wait for exchanges completion, in seconds
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 489:
> 487:
> 488: public void run() {
> 489: while (!isFinishing()) {
The name of the method is not great - but since it's pre-existing I'm OK with
that.
Suggestion:
// isFinishing() will be true when there are no active exchange
after terminating
while (!isFinishing()) {
test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java line 164:
> 162: // Complete the exchange 10 second into the future.
> 163: // Runs in parallel, so won't block the server stop
> 164: final Duration exchangeDuration =
> Duration.ofSeconds(Utils.adjustTimeout(5));
Let's use 10 since we're speaking about 10s everywhere. It doesn't really
matter since we should finish way before 10 (unless the bug is present).
Suggestion:
final Duration exchangeDuration =
Duration.ofSeconds(Utils.adjustTimeout(10));
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2102283879
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2102290292
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2102302993
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2102312019
