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

Wed Jun 4 13:16:17 UTC 2025

On Wed, 28 May 2025 11:26:17 GMT, p-nima <duke 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.

Changes requested by vyazici (Author).

@p-nima, thanks for quickly addressing the review feedback. I've dropped some further remarks.

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 34:

> 32:  * @build jdk.httpclient.test.lib.http2.Http2TestServer
> 33:  * @run junit HttpClientRetryLimitTest
> 34:  * @run junit/othervm -Djdk.httpclient.auth.retrylimit=1 HttpClientRetryLimitTest

@dfuch, shall we test against zero and negative values too? (Both are accepted by `AuthenticationFilter` as valid.)

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 ...

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.

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.

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 57:

> 55: 
> 56: 
> 57: class HttpClientRetryLimitTest implements HttpServerAdapters {

Shall we rename this to `HttpClientAuthRetryLimit`, since [there are several other retry limits](https://docs.oracle.com/en/java/javase/23/docs/api/java.net.http/module-summary.html#jdk.httpclient.auth.retrylimit)?

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 58:

> 56:     public void testDefaultSystemProperty() throws Exception {
> 57: 
> 58:         try (HttpTestServer httpTestServer = HttpTestServer.create(HttpClient.Version.HTTP_1_1)) {

Don't we need to verify the behavior for HTTP/2 too? If so, can we use a `@ParameterizedTest` instead and receive the protocol version? (There are several examples you can get inspired from; `HttpResponseConnectionLabelTest`, `EmptyAuthenticate`, etc.)

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 64:

> 62:             "jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
> 63: 
> 64:     static Stream<HttpClient.Version> args() {

I think we should also test against with SSL and without SSL cases. See `HttpResponseLimitingTest.ServerClientPair` for inspiration.

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?

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.

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 73:

> 71:     @ParameterizedTest
> 72:     @MethodSource("args")
> 73:     public void testDefaultSystemProperty(HttpClient.Version version) throws Exception {

I see you made the class package-private in 18bac9f. You could have additionally made the method package-private too.

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.

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 88:

> 86:             try (
> 87:                 HttpClient client = HttpClient.newBuilder()
> 88:                         .authenticator(new Authenticator() {

To ensure the client will fire the request using the protocol version of our preference, could you also pass `version` to the client builder too, please?

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.

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 99:

> 97:                 HttpRequest request = HttpRequest.newBuilder()
> 98:                         .GET()
> 99:                         .uri(new URI("http://" + httpTestServer.serverAuthority() + "/" + this.getClass().getSimpleName() + "/"))

To ensure the client will fire the request using the protocol version of our preference, could you also pass `version` to the request builder too, please?

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java line 113:

> 111:                 }
> 112:                 e.printStackTrace();
> 113:             }

AFAICT, you should be using `assertThrows` as follows:

IOException exception = assertThrows(...);
assertEquals(exception.message(), "too many authentication attempts. Limit: " + RETRY_LIMIT);
assertEquals(requestCount.get(), RETRY_LIMIT > 0 ? RETRY_LIMIT : 1);

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

PR Review: https://git.openjdk.org/jdk/pull/25490#pullrequestreview-2874776142
Changes requested by vyazici (Author).

PR Review: https://git.openjdk.org/jdk/pull/25490#pullrequestreview-2880603038
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111767554
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111701285
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111701781
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111702335
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2116087627
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111743245
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2116060516
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111722490
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111704822
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2115470192
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111730359
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2115451397
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2111760843
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2115449940
PR Review Comment: https://git.openjdk.org/jdk/pull/25490#discussion_r2115466187