RFR: 8377302: HttpServer::stop uses full timeout duration if handler throws
Daniel Fuchs
dfuchs at openjdk.org
Mon Feb 9 21:27:24 UTC 2026
On Mon, 9 Feb 2026 08:36:02 GMT, Volkan Yazici <vyazici 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.
>
> 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
Done
> 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;
> }
> }
> }
Well I had considered modifying WriteFinished and rejected it. I hadn't thought about replacing it with ExchangeFinished. Thanks for the suggestion.
> 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?
Done
> 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`?
I would prefer to keep it logically ordered. That is - the ref count is incremented at the point where we create the exchange, so the try-catch that ensures it's correctly decremented in case of any exception should begin at the line after that. We're invoking user code there (the handler) and we really want to catch things like AssertionError too. These things have a tendency to be just swallowed and disappear, leaving everything in an undefined state when they are fired from an executor thread. In addition - we do need to close the connection before firing the event - otherwise the connection state might get inconsistent by the time the event is handled, and new assertion errors will be fired down the line.
> 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?
HEAD is special because the body output stream is closed by sendResponseHeaders. So by the time the exception is thrown the body is already closed and the exchange is already finished. The expected behaviour is thus different depending on which HTTP method is used.
> 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? 😅
1. No - it might get GC'ed and the level configuration will be lost.
2. will do :-)
> 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`?
As I said, HEAD is special so I wanted to test that path too.
> 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.
Not sure having a time stamp would help much for diagnosability here.
> 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.
I don't see a reason to use `FailAndStopTest.class.getSimpleName()` rather than hardcoded "FailAndStopTest". In my experience when using copy/paste on the test this doesn't get handled by the IDE either way.
> 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?
Ah! Interestingly enough there are some discrepencies in behavior with https:
- some SocketException are transformed into IOException
- if there are bytes buffered in the output stream before the connection is closed, they are sent before closing the connection. This means that there are two cases where getInputStream().readAllBytes() throws with http but doesn't with https.
Seems like https was worth testing after all...
> 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.
Simplified
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783568436
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2782850684
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783569110
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781956401
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781984847
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781964666
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2781988450
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2782968292
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783585969
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783322890
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783574620
More information about the net-dev
mailing list