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