On Tue, 27 Jan 2026 15:41:56 GMT, Daniel Fuchs <[email protected]> wrote:
>> This fixes a deadlock between the thread that reads from the
>> RequestBodyInputStream and the thread that tries to close it in response to
>> a stream reset. See the linked JBS ticket for details.
>>
>> Tier1 and tier2 tests continue to pass. I verified that with this change
>> there are no busy threads at the end of the test.
>
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 320:
>
>> 318: close(io);
>> 319: var error = this.error;
>> 320: if (error != null) throw error;
>
> we should use the lock around these two lines.
I'll revisit that.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 329:
>
>> 327: if (closed) {
>> 328: throw new IOException("Stream is closed");
>> 329: }
>
> No sure we want that. If there is some unread data we want to read it first.
The InputStream spec states that `read` throws `IOException` if the stream was
closed. Note that we no longer set `closed = true` on error.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 334:
>
>> 332: if (closed) {
>> 333: throw new IOException("Stream is closed");
>> 334: }
>
> we should not do that if this.error != null.
I'd say that `closed` has a higher priority than `error`, because `closed`
means that the stream was closed by the user.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 358:
>
>> 356: var error = this.error;
>> 357: if (error == null) return -1;
>> 358: throw error;
>
> these lines should be protected by the lock.
That's not necessary. If we received an EOF, either the InputStream is closed,
or the QUIC stream is finished, or it is reset. The closed case was already
handled above, and EOF and reset are mutually exclusive.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 371:
>
>> 369: public void close() throws IOException {
>> 370: if (closed) return;
>> 371: closed = true;
>
> these lines should be protected by the lock.
I don't think it's needed. The worst thing that could happen is that we will
log twice.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http3/Http3ServerStreamImpl.java
> line 379:
>
>> 377: void close(IOException io) {
>> 378: if (error != null) return;
>> 379: error = io;
>
> close can be asynchronous. We should keep the lock here - or use synchronized.
I'll update the code so that these lines will only be called in response to
stream reset, once per stream, in a sequential scheduler.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2732995462
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2732997859
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2733011677
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2733022002
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2733025721
PR Review Comment: https://git.openjdk.org/jdk/pull/29448#discussion_r2733028936
