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

[Daniel Fuchs]

On Wed, 8 Jun 2022 18:29:10 GMT, Conor Cleary <ccleary at 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.

Good fix! Still some issues with the test.

src/java.net.http/share/classes/jdk/internal/net/http/ExchangeImpl.java line 255:

> 253: 
> 254:     // Called when server returns non 100 response to
> 255:     // and Expect-Continue

Typo?

test/jdk/java/net/httpclient/ExpectContinueTest.java line 89:

> 87:         InetSocketAddress saHang = new InetSocketAddress(InetAddress.getLoopbackAddress(), 0);
> 88: 
> 89:         httpTestServer = HttpServerAdapters.HttpTestServer.of(HttpServer.create(sa, 0));

You should be able to write:

httpTestServer = HttpTestServer.of(HttpServer.create(sa, 0));

here. No need for the `HttpServerAdapters` prefix.

test/jdk/java/net/httpclient/ExpectContinueTest.java line 217:

> 215:                         os.write(bytes);
> 216:                         os.flush();
> 217:                     }

maybe you should close the client socket if validRequest == false. You should also implement a close() method to close the server socket and the client socket when the test terminates. Also the test seems to be missing a teardown() method to properly close and stop all the servers.

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

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