RFR: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100 [v2]

[Jaikiran Pai]

On Thu, 9 Jun 2022 11:19:04 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line 862:
>> 
>>> 860:             // Sets a flag which closes the connection locally when
>>> 861:             // onFinished() is called
>>> 862:             response.closeWhenFinished();
>> 
>> I checked the HTTP/1.1 spec to see what it says about the `100-Continue` request/response flow. The spec (here https://www.rfc-editor.org/rfc/rfc7231#section-5.1.1) from a server perspective says that the server might either return a `417` or a final status code (or of course `100`). A 4xx is considered a client error and I think we don't close a connection on 4xx errors (I haven't looked thoroughly at the code yet to verify that part). So is there a reason why we would want to close a connection if the server responds with `417`? In fact, the spec says this for clients:
>> 
>> 
>> A client that receives a 417 (Expectation Failed) status code in
>> response to a request containing a 100-continue expectation SHOULD
>> repeat that request without a 100-continue expectation, since the
>> 417 response merely indicates that the response chain does not
>> support expectations (e.g., it passes through an HTTP/1.0 server).
>> 
>> 
>> It says `SHOULD`, so it isn't mandated that we retry the request on `417`.
>> 
>> Furthermore, the spec states that if the server sends a final status code instead of 417, then it might also send a `Connection: close` header to indicate whether or not the connection should be closed:
>> 
>> 
>> A server that responds with a final status code before reading the
>> entire message body SHOULD indicate in that response whether it
>> intends to close the connection or continue reading and discarding
>> the request message (see Section 6.6 of [RFC7230]).
>> 
>> 
>> Are we closing the connection irrespective of the status code (and other headers) to keep the implementation simpler? If that's the reason, then I think that's fine, since it still is within what the spec seems to allow. I'm just curious if there's some other reason for doing this.
>
> All good points. Yes - we could make a special case for 417 - and maybe we should log another issue to do that. We choose to close the connection in all cases here to make the implementation simpler. In practice I guess we could leave the connection opened - but if a server sends back anything but 100 then we might not be entirely sure of what state the server is in, so closing the connection seems safer.

Thank you for that explanation, Daniel. I agree it's reasonable decision to keep this simple.

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

PR: https://git.openjdk.java.net/jdk/pull/9093