On Wed, 28 May 2025 12:16:02 GMT, Volkan Yazici <[email protected]> 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
