On Wed, 28 May 2025 12:16:02 GMT, Volkan Yazici <vyaz...@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