RFR: 8054022: HttpURLConnection timeouts with Expect: 100-Continue and no chunking [v3]

[Daniel Fuchs]

On Thu, 27 Apr 2023 12:08:53 GMT, Darragh Clarke <duke at 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