On Thu, 9 Jun 2022 11:10:11 GMT, Jaikiran Pai <j...@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
