RFR: 8293786: HttpClient will not send more than 64 kb of data from the 2nd request in http2 [v4]

[Conor Cleary]

On Mon, 17 Apr 2023 10:28:47 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java line 737:
>> 
>>> 735:                 // the error code NO_ERROR will be sent to the client before closing.
>>> 736:                 if (bis instanceof BodyInputStream inputStream && (!inputStream.isEof() || inputStream.q.size() > 0)) {
>>> 737:                     bos.sendResetOnClose(ResetFrame.NO_ERROR);
>> 
>> I share @dfuch concern. This line will have no effect if `bos` is already closed at this point. We still need to reset the stream to let the peer know that we won't consume `bis`.
>> (edit) To illustrate that, try changing `PostPutTest.TestHandler.handle` to one of:
>> 
>>             exchange.sendResponseHeaders(200, -1);
>> 
>> or:
>> 
>>             exchange.sendResponseHeaders(200, 0);
>>             exchange.getResponseBody().close();
>> 
>> it will time out again.
>
> Thanks for the feedback Daniel. I see the issue that my implementation causes if the handler closes the `bos` early. Working on this now as a priority, solution will try to make sure that this RST_STREAM is sent even if the handler looks like the example you gave above. Probably worth inluding these handlers as test cases also.

So to address this in the most recent commit, the BodyOutputStream initalised in the try-with-resources block is now passed a reference to the given BodyInputStream and it is used to check if a reset is required there once it is closed. This is done with the following in BodyOutputStream.java:


private boolean resetNeeded() throws IOException {
        return (bis != null && (!bis.isEof() || bis.q.size() > 0));
}


For the `sendResponseHeaders(200, -1)` case, a reset has to be triggered to send instead once the BodyOutputStream is closed at the beginning of the exchange (because of the negative content length).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/12694#discussion_r1177999161