On Thu, 11 Dec 2025 12:16:52 GMT, Daniel Fuchs <[email protected]> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
>> line 1473:
>>
>>> 1471: numUnprocessed.incrementAndGet();
>>> 1472: } else {
>>> 1473: stream.connectionClosing(cause.getCloseCause());
>>
>> Hello @ehs208, I think this should be done a bit differently. Here we are
>> merely constructing a `TerminationCause` and then closing the streams with
>> that termination cause. But that doesn't guarantee that the connection will
>> have been closed with this termination cause. I think what we should do here
>> is merely mark the unprocessed streams with closeAsUnprocessed() and then
>> just call `close(Http2TerminationCause.forH2Error(errorCode, errorMsg);`.
>> The implementation of the `Http2Connection.Terminator.doTerminate()` already
>> has the necessary logic to close all these (processed) streams with the
>> termination cause. So I think this code should look something like:
>>
>> final byte[] debugData = frame.getDebugData();
>> final String debugInfo = debugData.length > 0
>> ? new String(debugData, UTF_8)
>> : "";
>>
>> ...
>>
>> streams.forEach((id, stream) -> {
>> if (id > lastProcessedStream) {
>> stream.closeAsUnprocessed();
>> numUnprocessed.incrementAndGet();
>> }
>> });
>> ...
>> // closes the connection as well as the rest of the (processed) streams
>> close(Http2TerminationCause.forH2Error(errorCode, errorMsg));
>>
>> I haven't tested this myself.
>
> @jaikiran won't the code at line 1483 below (`close(cause)`) ensure that the
> connection is actually closed?
Hello Daniel, line 1483, does guarantee that the connection will actually get
closed (if it isn't already).
In the current proposed form in this PR, the potential issue is that the
processed streams are being passed a termination cause (the one from goaway
frame), however the connection itself might get closed by a different cause, by
the time the close on line 1483 is run. So we can end up in a situation where
the connection closure cause is something else and the stream termination cause
is something else.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2610388687
