RFR: 8377302: HttpServer::stop uses full timeout duration if handler throws

[Volkan Yazici]

On Fri, 6 Feb 2026 13:54:06 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

> When a HttpHandler::handle method throws an unexpected exception, the HttpServer rightfully closes the associated connection. However, the exchange is still discounted as pending, which causes HttpServer::stop to wait for the full timeout duration, even though all connections have been closed.

Marked as reviewed by vyazici (Committer).

src/jdk.httpserver/share/classes/sun/net/httpserver/ExchangeImpl.java line 91:

> 89: 
> 90:     final AtomicBoolean ended = new AtomicBoolean();
> 91:     final AtomicBoolean finished = new AtomicBoolean();

*Nit:* I'd suggest preferring the minimum visibility, that is,

- `ended` and `finished` can be `private`
- `endExchange`, `postWriteFinished`, and `postExchangeFinished` can be package private

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 440:

> 438: 
> 439:             try {
> 440:                 if (r instanceof Event.WriteFinished || r instanceof Event.ExchangeFinished) {

This line is the only place in the code where `WriteFinished` and `ExchangeFinished` are read. Instead of creating two separate events with a big overlap, have you considered changing `WriteFinished` as follows:

    static final class ExchangeFinished extends Event {
        ExchangeFinished(ExchangeImpl t, boolean writeFinished) {
            super(Objects.requireNonNull(t));
            if (writeFinished) {
                assert !t.writefinished;
                t.writefinished = true;
            }
        }
    }

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 442:

> 440:                 if (r instanceof Event.WriteFinished || r instanceof Event.ExchangeFinished) {
> 441: 
> 442:                     logger.log(Level.TRACE, "Write Finished");

This log information is not accurate anymore — event can be `ExchangeFinished` too. I see a similar log line a couple of lines above right after the `r instanceof Event.StopRequested` check. Shall we instead remove both and add a `logger.log(Level.TRACE, "Handling {}", r.getClass().getSimpleName())` line at the beginning of the `handleEvent(Event)` method?

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 905:

> 903:                         uc.doFilter(new HttpExchangeImpl(tx));
> 904:                     }
> 905:                 } catch (Throwable t) {

1. I'm in favor of catching `Exception` instead of `Throwable` here, since the difference between the two are mostly irrecoverable `Error`s.
2. A couple of lines below, we have a `catch (Exception e)` block containing `closeConnection()`. Why can't we have `tx.postExchangeFinished()` there and remove the need for this new `try/catch`?

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 30:

> 28:  * @modules jdk.httpserver java.logging
> 29:  * @library /test/lib
> 30:  * @run main/othervm FailAndStopTest

A nice JTreg best practice:

Suggestion:

 * @run main/othervm ${test.main.class}

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 84:

> 82:                 default -> shouldFail && !"HEAD".equals(method);
> 83:             };
> 84:         }

We have a `shouldFail` flag, but we're introducing further logic to it in `shouldFail()` method. I _personally_ find it difficult to wrap my mind around it. Can we instead change the type of `shouldFail` from `boolean` to `Predicate<String>`? Or simplify this design some other way?

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 160:

> 158:     public static void main(String[] args) throws Exception {
> 159:         LOGGER.setLevel(Level.ALL);
> 160:         Logger.getLogger("").getHandlers()[0].setLevel(Level.ALL);

1. The `LOGGER` class constant is only used here. Can we move it here as a local variable?
2. Would you mind documenting these two lines for mere mortals who did not author JUL, please? 😅

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 165:

> 163:             test(test, Optional.empty());
> 164:         }
> 165:         // test with HEAD

What is the reason we repeat the test twice, once for `GET`, and once for `HEAD`?

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 175:

> 173:                 new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0);
> 174: 
> 175:         System.out.println("Test: " + method.orElse("GET") + " " + test.query);

AFAICT, there are time-sensitive asynchronous processing involved in the test. You might consider replacing `println("foo")` lines with `println(now() + " foo")` to help with troubleshooting. `now` can be statically imported from `java.util.time.Instant`, or you can roll-out your own `log(String format, Object... args)` method doing that.

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 178:

> 176:         System.out.println("Server listening at: " + server.getAddress());
> 177:         try {
> 178:             server.createContext("/FailAndStopTest/", new FailAndStopTest());

*Nit:* Rename resiliency?

Suggestion:

            server.createContext('/' + FailAndStopTest.class.getSimpleName() + '/', new FailAndStopTest());


Note that `.path("/FailAndStopTest/")` line below needs to be adapted too.

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 182:

> 180: 
> 181:             URL url = URIBuilder.newBuilder()
> 182:                     .scheme("http")

Would testing `HttpsServer` (i.e., the `https` scheme) be useful too?

test/jdk/com/sun/net/httpserver/FailAndStopTest.java line 200:

> 198:                             .formatted(test.query));
> 199:                 }
> 200:                 System.out.println("Client: read body: \"%s\"".formatted(body));

You might want to simplify `println("...".formatted(...))` with `printf`, in particular, if this needs to be backported.

-------------

PR Review: https://git.openjdk.org/jdk/pull/29606#pullrequestreview-3774798828
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781308125
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781366227
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781328367
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781402147
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2784018143
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781608044
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781529825
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781623523
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781568626
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781557562
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781574117
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781584621