On Sat, 5 Apr 2025 10:23:33 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
> Please help review this PR which improves `HttpServer::stop` termination
> timing to better fit user expectations.
>
> This PR:
>
> * Makes `ServerImpl::stop` continue without delay when there are no active
> exchanges during shutdown
> * Attempts to interrupt the dispatcher thread before joining it, giving
> long-living interruptible exchanges a chance to finish early
> * Adds testing for boundary conditions with or without an active exchange,
> delay occurring before exhange completes and exchange completing before delay.
>
> Additionally, `ServerImpl::stop` is updated to use `System::nanotime` instead
> of `System::currentTimeMillis` when calculating wait times.
>
> The test relies on timing to assert the order of events. Not perfect, but it
> seems to work.
>
> (This part of the code base is rather new to me. A bit of hand-holding should
> be expected when reviewing this PR)
src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 238:
> 236: selector.wakeup();
> 237: long latest = System.nanoTime() +
> Duration.ofSeconds(delay).toNanos();
> 238: while (activeExchanges() > 0 && System.nanoTime() < latest) {
Also the arithmetic is dubious since `System.nanoTime() +
Duration.ofSeconds(delay).toNanos();` could overflow. You never want to compare
`val1 < val2` - use `val2 - val1 > 0` instead.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24467#discussion_r2031352645
