On Tue, 4 Apr 2023 17:02:08 GMT, Darragh Clarke <d...@openjdk.org> wrote:
> Currently it is possible for `HttpURLConnection` with the `Expect: > 100-Continue` header to timeout awaiting for a server response. According to > [RFC-7231](https://www.rfc-editor.org/rfc/rfc7231#section-5.1.1) a client > `SHOULD NOT wait for an indefinite period before sending the message body`. > > This PR changes the existing `expect100Continue` method to wait for a maximum > of 5 seconds for a server response, this will be shorter if a timeout is set. > If no response is received, the message is sent regardless. > > Tests have been added to account for different scenarios that currently > timeout, and the changes have been tested against tiers 1,2 and 3. Changes requested by djelinski (Reviewer). src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java line 1370: > 1368: } > 1369: > 1370: http.setIgnoreContinue(true); Do you still need the `setIgnoreContinue(true)` in line 1339? src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java line 1453: > 1451: > 1452: if (expectContinue) { > 1453: http.setIgnoreContinue(false); Do you still need the `setIgnoreContinue(false)` a few lines above? test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 1: > 1: /* I noticed that you have tests for chunked streaming mode and for buffered mode; can you add tests for fixed length streaming mode as well? I.e. use `setFixedLengthStreamingMode`. test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 114: > 112: int contentLength = > Integer.parseInt(substr.trim()); > 113: > 114: if (control.respondWith100Continue) { this code is duplicated in the `if` and `else` branches; move it outside. ------------- PR Review: https://git.openjdk.org/jdk/pull/13330#pullrequestreview-1372246778 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1158113297 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1158084804 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1158245845 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1158237770
