On Thu, 9 Jun 2022 10:31:31 GMT, Conor Cleary <ccle...@openjdk.org> 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 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. src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 308: > 306: // Have to mark request as sent, due to no request body being > sent > 307: // in the event of a 417 Expectation Failed > 308: requestSent(); The `requestSent()` method in `Stream` class sets a flag and only closes the Stream instance if the `responseReceived` is also set. As far as I can see in the `Stream` code, that flag plays no other major role and doesn't "trigger" any kind of action (I might be wrong though). Here, are we expecting the caller client to get a request failure (just like the `HTTP/1.1` case)? If so, should we instead of calling `Stream.completeResponseExceptionally(Throwable)` or something similar which completes the `CompletableFuture`? ------------- PR: https://git.openjdk.java.net/jdk/pull/9093
