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

p-nima · 2025-05-28T11:26:17Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Error

⚠️ OCA signatory status must be verified

Issue

JDK-8340182: Java HttpClient does not follow default retry limit of 3 retries (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25490/head:pull/25490
$ git checkout pull/25490

Update a local copy of the PR:
$ git checkout pull/25490
$ git pull https://git.openjdk.org/jdk.git pull/25490/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25490

View PR using the GUI difftool:
$ git pr show -t 25490

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25490.diff

bridgekeeper · 2025-05-28T11:27:13Z

Hi @p-nima, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user p-nima" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-05-28T11:27:44Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-05-28T11:28:13Z

@p-nima The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

p-nima · 2025-05-28T11:40:30Z

/covered

bridgekeeper · 2025-05-28T11:41:23Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

myankelev · 2025-05-28T11:58:28Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+
+import static jdk.test.lib.Asserts.assertEquals;


minor: Did you mean import static org.junit.jupiter.api.Assertions.assertEquals; here, as you're using junit?

Thank you for your review. The changes have been made in 18bac9f

myankelev · 2025-05-28T12:04:31Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+public class HttpClientRetryLimitTest implements HttpServerAdapters {
+
+    private static final int DEFAULT_RETRY_LIMIT = 3;
+    private final int retryLimit = Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);


Nit: This feels a bit confusing to me when reading it the first time, why not have a retry limit of 1 or 0 as a default and then specify if you want more retries in @test?
I think it might be a bit easier to read, but if you want to keep it, it's fine with me.

This line is grabbing the value of the jdk.httpclient.auth.retrylimit system property, and if it hasn't been set then it defaults to 3. This is based on AuthenticationFilter.java

A comment has been added for it in cdf7812

myankelev · 2025-05-28T12:05:20Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+ * @library /test/lib /test/jdk/java/net/httpclient/lib
+ * @build jdk.httpclient.test.lib.http2.Http2TestServer
+ * @run junit HttpClientRetryLimitTest
+ * @run junit/othervm -Djdk.httpclient.auth.retrylimit=1 HttpClientRetryLimitTest


Do you think adding a retrylimit=0 would be beneficial too? This way every scenario would be covered

Added additional test case scenarios in 18bac9f

vy · 2025-05-28T12:16:02Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+
+import static jdk.test.lib.Asserts.assertEquals;
+
+public class HttpClientRetryLimitTest implements HttpServerAdapters {


Nit: JUnit recommends the following

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 18bac9f

vy · 2025-05-28T12:16:18Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+public class HttpClientRetryLimitTest implements HttpServerAdapters {
+
+    private static final int DEFAULT_RETRY_LIMIT = 3;
+    private final int retryLimit = Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);


This can be static too.

Updated this in 18bac9f

vy · 2025-05-28T12:16:33Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+
+    private static final int DEFAULT_RETRY_LIMIT = 3;
+    private final int retryLimit = Integer.getInteger("jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
+    private int countRetries;


This should better be converted to a local variable.

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

vy · 2025-05-28T12:17:57Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+            httpTestServer.addHandler(httpTestHandler, "/");
+            httpTestServer.start();
+
+            countRetries = 0;


IMHO, this should be an Atomic{Integer,Long} due to the possibility of concurrent updates.

Agreed. This has been updated in 18bac9f

vy · 2025-05-28T12:26:16Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+                t.sendResponseHeaders(401,0);
+            };
+
+            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 18bac9f

@p-nima, the handler still accepts all calls to /, you only salted the request URI. Would you mind doing the same in httpTestServer.addHandler(...) too, please? (You better create a String requestPath variable and use it both at the handler and the request.)

vy · 2025-05-28T12:29:09Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+                        .uri(new URI("http://" + httpTestServer.serverAuthority() + "/"))
+                        .build();
+
+                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 18bac9f

vy · 2025-05-28T12:34:44Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+    @Test
+    public void testDefaultSystemProperty() throws Exception {
+
+        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.)

vy · 2025-05-28T12:43:21Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+            } catch (Exception e) {
+                assertEquals(retryLimit, countRetries,
+                        "Expected number of retries was " + retryLimit + " but actual was "+countRetries);
+                e.printStackTrace();
+            }


AFAIU, you should

Wrap client::send with assertThrows

Verify the returned exception

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 18bac9f

vy · 2025-05-28T12:46:25Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+ * @library /test/lib /test/jdk/java/net/httpclient/lib
+ * @build jdk.httpclient.test.lib.http2.Http2TestServer
+ * @run junit HttpClientRetryLimitTest
+ * @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.)

We could. We could also throw in garbage to see what happens.

New test scenarios have been added in 18bac9f. We have default, positive, zero and negative values.

vy

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

vy · 2025-05-30T08:57:47Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+                t.sendResponseHeaders(401,0);
+            };
+
+            httpTestServer.addHandler(httpTestHandler, "/");


@p-nima, the handler still accepts all calls to /, you only salted the request URI. Would you mind doing the same in httpTestServer.addHandler(...) too, please? (You better create a String requestPath variable and use it both at the handler and the request.)

vy · 2025-05-30T08:58:23Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+
+                HttpRequest request = HttpRequest.newBuilder()
+                        .GET()
+                        .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?

vy · 2025-05-30T08:59:00Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+            httpTestServer.start();
+            try (
+                HttpClient client = HttpClient.newBuilder()
+                        .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?

vy · 2025-05-30T09:06:00Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+                throw assertThrows(IOException.class, () -> client.send(request, HttpResponse.BodyHandlers.discarding()));
+            } catch (Exception e) {
+                if (RETRY_LIMIT > 0){
+                    assertEquals(RETRY_LIMIT, requestCount.get(),
+                            "Expected number of request retries was " + RETRY_LIMIT + " but actual was "+requestCount);
+                }
+                else{
+                    requestCount.decrementAndGet();
+                    assertEquals(0, requestCount.get(),
+                            "Expected number of request retries was 0 but actual was "+requestCount);
+                }
+                e.printStackTrace();
+            }


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);

vy · 2025-05-30T09:08:46Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+
+    @ParameterizedTest
+    @MethodSource("args")
+    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.

vy · 2025-05-30T14:42:11Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+    private static final int RETRY_LIMIT = Integer.getInteger(
+            "jdk.httpclient.auth.retrylimit", DEFAULT_RETRY_LIMIT);
+
+    static Stream<HttpClient.Version> args() {


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

vy · 2025-05-30T14:57:38Z

test/jdk/java/net/httpclient/HttpClientRetryLimitTest.java

+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+
+class HttpClientRetryLimitTest implements HttpServerAdapters {


Shall we rename this to HttpClientAuthRetryLimit, since there are several other retry limits?

Sounds reasonable to rename - and fix the @summary in the jtreg headers too

jaikiran · 2025-05-30T16:01:12Z

/touch

openjdk · 2025-05-30T16:01:51Z

@jaikiran The pull request is being re-evaluated and the inactivity timeout has been reset.

fix for the http client default retry limit

e89aa7c

bridgekeeper bot added the oca Needs verification of OCA signatory status label May 28, 2025

openjdk bot added the net net-dev@openjdk.org label May 28, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label May 28, 2025

myankelev reviewed May 28, 2025

View reviewed changes

vy suggested changes May 28, 2025

View reviewed changes

p-nima added 2 commits May 29, 2025 15:45

apply review feedback

18bac9f

add comment to indicate system default value

cdf7812

vy suggested changes May 30, 2025

View reviewed changes

vy reviewed May 30, 2025

View reviewed changes


		import static jdk.test.lib.Asserts.assertEquals;

		public class HttpClientRetryLimitTest implements HttpServerAdapters {

		import static org.junit.jupiter.api.Assertions.assertThrows;


		class HttpClientRetryLimitTest implements HttpServerAdapters {

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

p-nima commented May 28, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Error

Issue

Reviewing

Uh oh!

bridgekeeper bot commented May 28, 2025

Uh oh!

openjdk bot commented May 28, 2025

Uh oh!

openjdk bot commented May 28, 2025

Uh oh!

p-nima commented May 28, 2025

Uh oh!

bridgekeeper bot commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

p-nima commented May 28, 2025 •

edited by openjdk bot

Loading