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

[Daniel Fuchs]

On Thu, 9 Jun 2022 11:10:11 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8286171: Added teardown method and comments
>
> 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.

> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line 424:
> 
>> 422:     }
>> 423: 
>> 424:     public void closeWhenFinished() {
> 
> Hello Conor, do you think it might be better if we make this package private access instead of `public`?

Yes - good catch!

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

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