On Mon, 9 Feb 2026 08:36:02 GMT, Volkan Yazici <[email protected]> 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
