On Thu, 27 Apr 2023 12:08:53 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. > > Darragh Clarke has updated the pull request incrementally with one additional > commit since the last revision: > > Made the individual variables in Control volatile instead of the class > declaration itself Since control can be accessed from different threads it's better to make it volatile too - and then stuck it in a local variable before reading its fields. test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 61: > 59: > 60: private Thread serverThread = null; > 61: private Control control = null; Suggestion: private volatile Control control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 72: > 70: @BeforeAll > 71: public void startServerSocket() throws Exception { > 72: control = new Control(); Suggestion: Control control = this.control = new Control(); test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 177: > 175: control.stop = true; > 176: control.serverSocket.close(); > 177: serverThread.join(); Suggestion: Control control = this.control; control.stop = true; control.serverSocket.close(); serverThread.join(); test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 182: > 180: @Test > 181: public void testNonChunkedRequestAndNoExpect100ContinueResponse() > throws Exception { > 182: String body = > "testNonChunkedRequestAndNoExpect100ContinueResponse"; Suggestion: String body = "testNonChunkedRequestAndNoExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 207: > 205: @Test > 206: public void testNonChunkedRequestWithExpect100ContinueResponse() > throws Exception { > 207: String body = > "testNonChunkedRequestWithExpect100ContinueResponse"; Suggestion: String body = "testNonChunkedRequestWithExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 232: > 230: @Test > 231: public void > testNonChunkedRequestWithDoubleExpect100ContinueResponse() throws Exception { > 232: String body = > "testNonChunkedRequestWithDoubleExpect100ContinueResponse"; Suggestion: String body = "testNonChunkedRequestWithDoubleExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 257: > 255: @Test > 256: public void testChunkedRequestAndNoExpect100ContinueResponse() > throws Exception { > 257: String body = "testChunkedRequestAndNoExpect100ContinueResponse"; Suggestion: String body = "testChunkedRequestAndNoExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 283: > 281: @Test > 282: public void testChunkedRequestWithExpect100ContinueResponse() throws > Exception { > 283: String body = "testChunkedRequestWithExpect100ContinueResponse"; Suggestion: String body = "testChunkedRequestWithExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 309: > 307: @Test > 308: public void testChunkedRequestWithDoubleExpect100ContinueResponse() > throws Exception { > 309: String body = > "testChunkedRequestWithDoubleExpect100ContinueResponse"; Suggestion: String body = "testChunkedRequestWithDoubleExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 335: > 333: @Test > 334: public void testFixedLengthRequestAndNoExpect100ContinueResponse() > throws Exception { > 335: String body = > "testFixedLengthRequestAndNoExpect100ContinueResponse"; Suggestion: String body = "testFixedLengthRequestAndNoExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 361: > 359: @Test > 360: public void testFixedLengthRequestWithExpect100ContinueResponse() > throws Exception { > 361: String body = > "testFixedLengthRequestWithExpect100ContinueResponse"; Suggestion: String body = "testFixedLengthRequestWithExpect100ContinueResponse"; Control control = this.control; test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java line 387: > 385: @Test > 386: public void > testFixedLengthRequestWithDoubleExpect100ContinueResponse() throws Exception { > 387: String body = > "testFixedLengthRequestWithDoubleExpect100ContinueResponse"; Suggestion: String body = "testFixedLengthRequestWithDoubleExpect100ContinueResponse"; Control control = this.control; ------------- PR Review: https://git.openjdk.org/jdk/pull/13330#pullrequestreview-1410611920 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183508706 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183509329 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183510690 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183511557 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183512339 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183512846 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183513668 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183514365 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183514649 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183514895 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183515111 PR Review Comment: https://git.openjdk.org/jdk/pull/13330#discussion_r1183515310
