On Wed, 14 Sep 2022 13:57:34 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to fix >> https://bugs.openjdk.org/browse/JDK-8292044? >> >> The linked JBS issue notes two parts to fixing this. Part one is to >> (internally) ignore the intermediate 1xx informational responses, in the >> client and wait for subsequent final response from the server. Part two is >> to introduce newer APIs to let applications using HttpClient, to have access >> to these intermediate response (codes). This commit (only) addresses part >> one. Part two is out of scope of this change and a separate issue will be >> opened to address it (at a later time). >> >> The commit in this PR introduces a check to see if the returned response is >> an informational response (as defined by RFC-2616 >> https://www.rfc-editor.org/rfc/rfc2616#page-58). If the response code is >> between 102 and 199 (inclusive), then this change ignores that response and >> keeps waiting for a subsequent final response from the server. >> >> The request timeout (if set) will _not_ be reset when a intermediate >> informational response is received (and we ignore it). The request timeout >> handling continues to be the same as what it is currently and will span from >> the request start till the final response is received. If no final response >> is received within the duration of request timeout (if set) then the >> application will continue to receive a request timeout exception. >> >> A new test class has been introduced to reproduce the issue and test the >> fix. The test tests both HTTP/1.1 and HTTP2. >> >> tier1, tier2 and tier3 testing is in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Close the connection/stream when a 101 response isn't expected Changes requested by dfuchs (Reviewer). src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line 506: > 504: > 505: @Override > 506: void onProtocolError(final String errorMessage) { Should take the IOException/ProtocolException as parameter and call cancelImpl(IOException). cancelImpl should do the right thing. src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1242: > 1240: > 1241: @Override > 1242: void onProtocolError(final String errorMessage) { I believe this should take an `IOException` or `ProtocolException` as parameter and call `cancelImpl(IOException)`. It should take care of resetting the stream if needed, and properly account for the ref counting of inflight operation. test/jdk/java/net/httpclient/Response1xxTest.java line 71: > 69: > 70: > 71: private HttpServerAdapters.HttpTestServer http2Server; // h2c can we remove the outer class from the declaration? test/jdk/java/net/httpclient/Response1xxTest.java line 76: > 74: > 75: private SSLContext sslContext; > 76: private HttpServerAdapters.HttpTestServer https2Server; // h2 same here? test/jdk/java/net/httpclient/Response1xxTest.java line 133: > 131: https2Server.stop(); > 132: System.out.println("Stopped (https) HTTP2 server"); > 133: } This test should probably use a `final ReferenceTracker TRACKER = ReferenceTracker.INSTANCE` now, to double check that all clients have properly terminated without any dangling operations at the end of the test. You can `grep TRACKER` in existing HttpClient tests to see how this is used. The TRACKER can be used to track created clients, and invoked at the end of the test to verify that all resources have been properly reclaimed. ------------- PR: https://git.openjdk.org/jdk/pull/10169
