On Thu, 9 Jun 2022 11:27:47 GMT, Conor Cleary <[email protected]> wrote:
>> **Issue**
>> It was observed that when the httpclient sends a POST request with the
>> `Expect: 100 Continue` header set and the server replies with a response
>> code `417 Expectation Failed` that the httpclient hangs indefinitely when
>> the version of Http used is HTTP/2. However, it was also seen that the issue
>> persisted with HTTP/1_1 with the same usage.
>>
>> This was caused by an implementation in ExchangeImpl that resulted in two
>> calls to readBodyAsync() in this case, where the second call causes the
>> indefinite hanging (as if there was a respomse body, it has already been
>> read).
>>
>> **Solution**
>> When ExchangeImpl::expectContinue() detects that a response code 417 is
>> received, two things occur. Firstly, a flag is set which ensures that the
>> connection is closed locally in this case. Secondly, the response is
>> returned to the client as a failed future, A unit test was added to ensure
>> that this usage of the httpclient does not cause hanging.
>
> Conor Cleary has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8286171: Package-protected access for method
> - 8286171: Added checks for correct response codes
test/jdk/java/net/httpclient/ExpectContinueTest.java line 131:
> 129: @Override
> 130: public void handle(HttpTestExchange exchange) throws IOException
> {
> 131: try (InputStream is = exchange.getRequestBody();
Should this also be doing something similar to what the `PostHandler` does for
HTTP2 versioned request?
test/jdk/java/net/httpclient/ExpectContinueTest.java line 165:
> 163: public void handle(HttpTestExchange exchange) throws IOException
> {
> 164: //Send 417 Headers, tell client to not send body
> 165: try (InputStream is = exchange.getRequestBody();
Should we include a couple of other variants of the server to cover some other
possible flows. For example:
- Don't read the request body, but just send the 417 response header?
- Send some other final response status code other than 100 or 417?
test/jdk/java/net/httpclient/ExpectContinueTest.java line 264:
> 262:
> 263: HttpRequest getRequest = HttpRequest.newBuilder(getUri)
> 264: .GET()
Is this missing an `.expectContinue(true)` here?
-------------
PR: https://git.openjdk.java.net/jdk/pull/9093
