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

[Daniel Fuchs]

On Mon, 9 Feb 2026 14:08:30 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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