On Mon, 9 Feb 2026 14:08:30 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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.
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.
WontFix ;-)
>> 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.
Done. Added predicates.
>> 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.
WontFix...
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783570401
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783571734
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783575747
PR Review Comment: https://git.openjdk.org/jdk/pull/29606#discussion_r2783572970
