RFR: 8340182: Java HttpClient does not follow default retry limit of 3 retries

[p-nima]

On Wed, 28 May 2025 12:16:02 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> The AuthenticationFilter did not respect the default retry limit of 3 retries in case of invalid credentials supplied.
>> 
>> This PR helps to resolve the bug and tests it with default and updated retry limit set via `jdk.httpclient.auth.retrylimit=1`.
>> 
>> The test is green with tiers 1, 2, 3 and the test is stable.
>
> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 49:
> 
>> 47: import static jdk.test.lib.Asserts.assertEquals;
>> 48: 
>> 49: public class HttpClientRetryLimitTest implements HttpServerAdapters {
> 
> *Nit:* [JUnit recommends the following](https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-classes-and-methods)
>> It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so ...

Thank you for your feedback. The public modifier has been discarded in 18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 52:
> 
>> 50: 
>> 51:     private static final int DEFAULT_RETRY_LIMIT = 3;
>> 52:     private final int retryLimit = Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
> 
> This can be `static` too.

Updated this in https://github.com/openjdk/jdk/commit/18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 53:
> 
>> 51:     private static final int DEFAULT_RETRY_LIMIT = 3;
>> 52:     private final int retryLimit = Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
>> 53:     private int countRetries;
> 
> This should better be converted to a local variable.

Discarded the above local variable and introduced a AtomicInteger variable in  18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 66:
> 
>> 64:             };
>> 65: 
>> 66:             httpTestServer.addHandler(httpTestHandler, "/");
> 
> In the past, there has been occasions in CI where a test server received connections from a totally unrelated test running in parallel on the same host. To avoid such mishaps, we better salt the path a bit by, say, appending the class' simple name?

Thanks for the catch - the class simple name has been appended in 18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 69:
> 
>> 67:             httpTestServer.start();
>> 68: 
>> 69:             countRetries = 0;
> 
> IMHO, this should be an `Atomic{Integer,Long}` due to the possibility of concurrent updates.

Agreed. This has been updated in 18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 87:
> 
>> 85:                         .build();
>> 86: 
>> 87:                 client.send(request, HttpResponse.BodyHandlers.ofString());
> 
> Since we're not interested in the response, `BodyHandlers.discarding()` might be a better fit here.

Updated this in 18bac9fa64f81110c2894f5f141e88ec5dc20b03

> test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 92:
> 
>> 90:                         "Expected number of retries was " + retryLimit + " but actual was "+countRetries);
>> 91:                 e.printStackTrace();
>> 92:             }
> 
> AFAIU, you should
> 
> 1. Wrap `client::send` with `assertThrows`
> 2. Verify the returned exception
> 1. Then `assertEquals` the retry count
> 
> Otherwise, if `client::send` _unexpectedly_ doesn't throw anything, your test will incorrectly succeed.

Yes, agreed - the changes have been made in 18bac9fa64f81110c2894f5f141e88ec5dc20b03

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114266126
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114275602
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114269419
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114274151
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114270266
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114274886
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2114298224