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) {
I don't think that works unfortunately. We need something that takes into
account that an exchange may have been started but hasn't reached the point
where startExchange has been called.
The problem is that exchangeCount is incremeted asynchronously in the
ExchangeImpl constructor, and the ExchangeImpl is created by the Exchange::run
method - which is run asynchronoously in the executor (submitted by the
dispacher thread).
Possibly posting a StopRequested event to the dispatcher thread, and have the
dispacther thread switch finished = true when is sees the StopRequested event
and notices that allConnections only contains idleConnections (in which case we
could also assert that exchangeCount == 0).
Unless I'm not mistaken this is not going to be a trivial fix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24467#discussion_r2031318272
