From 42bcec6653aa3abb847ac0af2720be75c7209004 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 25 Jun 2025 17:02:35 +0200 Subject: [PATCH 01/12] test: verify valid client request context after retrying test --- .../client/retry/RetryingClientTest.java | 308 ++++++++++++++---- 1 file changed, 238 insertions(+), 70 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index c68446ad7e3..05fd46654df 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -21,6 +21,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.catchThrowable; +import static org.assertj.core.api.Assertions.fail; +import static org.awaitility.Awaitility.await; import java.time.Duration; import java.util.Arrays; @@ -31,6 +33,7 @@ import java.util.concurrent.CompletionStage; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -55,6 +58,8 @@ import com.linecorp.armeria.client.ClientFactory; import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.client.ClientRequestContextCaptor; +import com.linecorp.armeria.client.Clients; import com.linecorp.armeria.client.HttpClient; import com.linecorp.armeria.client.ResponseTimeoutException; import com.linecorp.armeria.client.UnprocessedRequestException; @@ -102,6 +107,9 @@ static void afterAll() { clientFactory.closeAsync(); } + @Nullable + ClientRequestContext ctx; + private final AtomicInteger responseAbortServiceCallCounter = new AtomicInteger(); private final AtomicInteger requestAbortServiceCallCounter = new AtomicInteger(); @@ -328,16 +336,25 @@ void retryWhenContentMatched() { .factory(clientFactory) .decorator(retryingDecorator) .build(); - - final AggregatedHttpResponse res = client.get("/retry-content").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/retry-content").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx, 3); } @Test void retryWhenStatusMatched() { final WebClient client = client(RetryRule.builder().onServerErrorStatus().onException().thenBackoff()); - final AggregatedHttpResponse res = client.get("/503-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/503-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -346,8 +363,13 @@ void retryWhenStatusMatchedWithContent() { .onServerErrorStatus() .onException() .thenBackoff(), 10000, 0, 100); - final AggregatedHttpResponse res = client.get("/503-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/503-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -358,8 +380,13 @@ void retryWhenTrailerMatched() { return trailers.getInt("grpc-status", -1) != 0; }) .thenBackoff()); - final AggregatedHttpResponse res = client.get("/trailers-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/trailers-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -368,16 +395,26 @@ void retryWhenTotalDurationIsHigh() { client(RetryRule.builder() .onTotalDuration((unused, duration) -> duration.toNanos() > 100) .thenBackoff()); - final AggregatedHttpResponse res = client.get("/1sleep-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/1sleep-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx); } @Test void disableResponseTimeout() { final WebClient client = client(RetryRule.failsafe(), 0, 0, 100); - final AggregatedHttpResponse res = client.get("/503-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/503-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // response timeout did not happen. + awaitValidClientRequestContext(ctx, 2); } @Test @@ -385,10 +422,16 @@ void respectRetryAfter() { final WebClient client = client(RetryRule.failsafe()); final Stopwatch sw = Stopwatch.createStarted(); - final AggregatedHttpResponse res = client.get("/retry-after-1-second").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/retry-after-1-second").aggregate().join(); + ctx = captor.get(); + } + assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo( (long) (TimeUnit.SECONDS.toMillis(1) * 0.9)); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -396,12 +439,17 @@ void respectRetryAfterWithHttpDate() { final WebClient client = client(RetryRule.failsafe()); final Stopwatch sw = Stopwatch.createStarted(); - final AggregatedHttpResponse res = client.get("/retry-after-with-http-date").aggregate().join(); - assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/retry-after-with-http-date").aggregate().join(); + ctx = captor.get(); + } + assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // Since ZonedDateTime doesn't express exact time, // just check out whether it is retried after delayed some time. assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo(1000); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -410,27 +458,41 @@ void propagateLastResponseWhenNextRetryIsAfterTimeout() { .onServerErrorStatus() .onException() .thenBackoff(Backoff.fixed(10000000))); - final AggregatedHttpResponse res = client.get("/service-unavailable").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/service-unavailable").aggregate().join(); + ctx = captor.get(); + } assertThat(res.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); + awaitValidClientRequestContext(ctx, 1); } @Test void propagateLastResponseWhenExceedMaxAttempts() { final WebClient client = client( RetryRule.builder().onServerErrorStatus().onException().thenBackoff(Backoff.fixed(1)), 0, 0, 3); - final AggregatedHttpResponse res = client.get("/service-unavailable").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/service-unavailable").aggregate().join(); + ctx = captor.get(); + } assertThat(res.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); + awaitValidClientRequestContext(ctx, 3); // equal to max attempts } @Test void retryAfterOneYear() { final WebClient client = client(RetryRule.failsafe()); - // The response will be the last response whose headers contains HttpHeaderNames.RETRY_AFTER // because next retry is after timeout - final ResponseHeaders headers = client.get("retry-after-one-year").aggregate().join().headers(); + final ResponseHeaders headers; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + headers = client.get("retry-after-one-year").aggregate().join().headers(); + ctx = captor.get(); + } assertThat(headers.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); assertThat(headers.get(HttpHeaderNames.RETRY_AFTER)).isNotNull(); + awaitValidClientRequestContext(ctx, 1); } @Test @@ -445,8 +507,15 @@ void retryOnResponseTimeout() { }; final WebClient client = client(strategy, 0, 500, 100); - final AggregatedHttpResponse res = client.get("/1sleep-then-success").aggregate().join(); + + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/1sleep-then-success").aggregate().join(); + ctx = captor.get(); + } + assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -467,10 +536,15 @@ void retryWithContentOnResponseTimeout() { .onException(ResponseTimeoutException.class) .thenBackoff(backoff))); final WebClient client = client(strategy, 0, 500, 100); - final AggregatedHttpResponse res = client.get("/1sleep-then-success").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/1sleep-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // Make sure that all customized RetryRuleWithContents are called. assertThat(queue).containsExactly(1, 2, 3); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -507,49 +581,69 @@ void honorRetryMapping() { final WebClient client = client(mapping); Stopwatch stopwatch = Stopwatch.createStarted(); - assertThat(client.get("/500-always").aggregate().join().status()) - .isEqualTo(HttpStatus.valueOf(500)); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.get("/500-always").aggregate().join().status()) + .isEqualTo(HttpStatus.valueOf(500)); + ctx = captor.get(); + } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(2), Duration.ofSeconds(6)); + awaitValidClientRequestContext(ctx, 2); stopwatch = Stopwatch.createStarted(); - assertThat(client.get("/501-always").aggregate().join().status()) - .isEqualTo(HttpStatus.valueOf(501)); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.get("/501-always").aggregate().join().status()) + .isEqualTo(HttpStatus.valueOf(501)); + ctx = captor.get(); + } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(14), Duration.ofSeconds(28)); + awaitValidClientRequestContext(ctx, 8); stopwatch = Stopwatch.createStarted(); - assertThat(client.get("/502-always").aggregate().join().status()) - .isEqualTo(HttpStatus.valueOf(502)); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.get("/502-always").aggregate().join().status()) + .isEqualTo(HttpStatus.valueOf(502)); + ctx = captor.get(); + } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(0), Duration.ofSeconds(2)); + awaitValidClientRequestContext(ctx, 1); } @Test void evaluatesMappingOnce() { final AtomicInteger evaluations = new AtomicInteger(0); final RetryConfigMapping mapping = - (ctx, req) -> { - evaluations.incrementAndGet(); - return RetryConfig - .builder0(RetryRule.builder() - .onStatus(HttpStatus.valueOf(500)) - .thenBackoff()) - .maxTotalAttempts(2) - .build(); - }; + (ctx, req) -> { + evaluations.incrementAndGet(); + return RetryConfig + .builder0(RetryRule.builder() + .onStatus(HttpStatus.valueOf(500)) + .thenBackoff()) + .maxTotalAttempts(2) + .build(); + }; final WebClient client = client(mapping); - assertThat(client.get("/500-then-success").aggregate().join().status()) - .isEqualTo(HttpStatus.valueOf(200)); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.get("/500-then-success").aggregate().join().status()) + .isEqualTo(HttpStatus.valueOf(200)); + ctx = captor.get(); + } // 1 logical request; 2 retries assertThat(evaluations.get()).isEqualTo(1); + awaitValidClientRequestContext(ctx, 2); reqCount.set(0); - assertThat(client.get("/500-then-success").aggregate().join().status()) - .isEqualTo(HttpStatus.valueOf(200)); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.get("/500-then-success").aggregate().join().status()) + .isEqualTo(HttpStatus.valueOf(200)); + ctx = captor.get(); + } - // 2 logical requests; 4 retries + // 2 logical requests; 2 retries assertThat(evaluations.get()).isEqualTo(2); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -582,10 +676,14 @@ void retryWithContentOnUnprocessedException() { .decorator(retryingDecorator) .build(); final Stopwatch stopwatch = Stopwatch.createStarted(); - assertThatThrownBy(() -> client.get("/unprocessed-exception").aggregate().join()) - .isInstanceOf(CompletionException.class) - .hasCauseInstanceOf(UnprocessedRequestException.class); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(() -> client.get("/unprocessed-exception").aggregate().join()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(UnprocessedRequestException.class); + ctx = captor.get(); + } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(7), Duration.ofSeconds(20)); + awaitValidClientRequestContext(ctx, 5); // max attempts } } @@ -594,17 +692,26 @@ void retryWithContentOnUnprocessedException() { void differentBackoffBasedOnStatus(RetryRule retryRule) { final WebClient client = client(retryRule); + AggregatedHttpResponse res; final Stopwatch sw = Stopwatch.createStarted(); - AggregatedHttpResponse res = client.get("/503-then-success").aggregate().join(); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/503-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isBetween((long) (10 * 0.9), (long) (1000 * 1.1)); + awaitValidClientRequestContext(ctx, 2); reqCount.set(0); sw.reset().start(); - res = client.get("/500-then-success").aggregate().join(); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.get("/500-then-success").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo((long) (1000 * 0.9)); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -613,8 +720,13 @@ void retryWithRequestBody() { .onServerErrorStatus() .onException() .thenBackoff(Backoff.fixed(10))); - final AggregatedHttpResponse res = client.post("/post-ping-pong", "bar").aggregate().join(); + final AggregatedHttpResponse res; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + res = client.post("/post-ping-pong", "bar").aggregate().join(); + ctx = captor.get(); + } assertThat(res.contentUtf8()).isEqualTo("bar"); + awaitValidClientRequestContext(ctx, 2); } @Test @@ -652,7 +764,12 @@ void shouldGetExceptionWhenFactoryIsClosed() { // // Peel CompletionException first. - Throwable t = peel(catchThrowable(() -> client.get("/service-unavailable").aggregate().join())); + Throwable t; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + t = peel(catchThrowable(() -> client.get("/service-unavailable").aggregate().join())); + ctx = captor.get(); + } + awaitValidClientRequestContext(ctx, 1); // not able to schedule second retry. if (t instanceof UnprocessedRequestException) { final Throwable cause = t.getCause(); assertThat(cause).isInstanceOf(IllegalStateException.class); @@ -679,12 +796,17 @@ void doNotRetryWhenResponseIsAborted() throws Exception { .decorator(LoggingClient.newDecorator()) .build(); responseAbortServiceCallCounter.set(0); - final HttpResponse httpResponse = client.get("/response-abort"); + final HttpResponse httpResponse; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + httpResponse = client.get("/response-abort"); + ctx = captor.get(); + } if (abortCause == null) { httpResponse.abort(); } else { httpResponse.abort(abortCause); } + awaitValidClientRequestContext(ctx, 1); final RequestLog log = context.get().log().whenComplete().join(); final Throwable requestCause = log.requestCause(); @@ -712,23 +834,28 @@ void doNotRetryWhenResponseIsAborted() throws Exception { @Test void doNotRetryWhenSubscriberIsCancelled() throws Exception { final WebClient client = client(retryAlways); - client.get("/subscriber-cancel").subscribe( - new Subscriber() { - @Override - public void onSubscribe(Subscription s) { - s.cancel(); // Cancel as soon as getting the subscription. - } - @Override - public void onNext(HttpObject httpObject) {} + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + client.get("/subscriber-cancel").subscribe( + new Subscriber() { + @Override + public void onSubscribe(Subscription s) { + s.cancel(); // Cancel as soon as getting the subscription. + } + + @Override + public void onNext(HttpObject httpObject) {} - @Override - public void onError(Throwable t) {} + @Override + public void onError(Throwable t) {} - @Override - public void onComplete() {} - }); + @Override + public void onComplete() {} + }); + ctx = captor.get(); + } + awaitValidClientRequestContext(ctx, 1); TimeUnit.SECONDS.sleep(1L); // Sleep to check if there's a retry. assertThat(subscriberCancelServiceCallCounter.get()).isEqualTo(1); } @@ -755,11 +882,15 @@ void doNotRetryWhenRequestIsAborted() throws Exception { } else { req.abort(abortCause); } - client.execute(req).aggregate(); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + client.execute(req).aggregate(); + ctx = captor.get(); + } TimeUnit.SECONDS.sleep(1); // No request is made. assertThat(responseAbortServiceCallCounter.get()).isZero(); + awaitValidClientRequestContext(ctx, 0); final RequestLog log = context.get().log().whenComplete().join(); if (abortCause == null) { assertThat(log.requestCause()).isExactlyInstanceOf(AbortedStreamException.class); @@ -785,9 +916,14 @@ void exceptionInDecorator() { .decorator(RetryingClient.newDecorator(strategy, 5)) .build(); - assertThatThrownBy(() -> client.get("/").aggregate().join()) - .hasCauseExactlyInstanceOf(AnticipatedException.class); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(() -> client.get("/").aggregate().join()) + .isInstanceOf(CompletionException.class) + .hasCauseExactlyInstanceOf(AnticipatedException.class); + ctx = captor.get(); + } assertThat(retryCounter.get()).isEqualTo(5); + awaitValidClientRequestContext(ctx, 5); } @Test @@ -798,9 +934,13 @@ void exceptionInRule() { }; final WebClient client = client(rule); - assertThatThrownBy(client.get("/").aggregate()::join) - .isInstanceOf(CompletionException.class) - .hasCauseReference(exception); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(client.get("/").aggregate()::join) + .isInstanceOf(CompletionException.class) + .hasCauseReference(exception); + ctx = captor.get(); + } + awaitValidClientRequestContext(ctx, 1); } @Test @@ -811,9 +951,13 @@ void exceptionInRuleWithContent() { }; final WebClient client = client(rule, 10000, 0, 100); - assertThatThrownBy(client.get("/").aggregate()::join) - .isInstanceOf(CompletionException.class) - .hasCauseReference(exception); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(client.get("/").aggregate()::join) + .isInstanceOf(CompletionException.class) + .hasCauseReference(exception); + ctx = captor.get(); + } + awaitValidClientRequestContext(ctx, 1); } @Test @@ -828,11 +972,15 @@ void useSameEventLoopWhenAggregate() throws InterruptedException { }) .decorator(RetryingClient.newDecorator(RetryRule.failsafe(), 2)) .build(); - client.get("/503-then-success").aggregate().whenComplete((unused, cause) -> { - assertThat(eventLoop.get().inEventLoop()).isTrue(); - latch.countDown(); - }); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + client.get("/503-then-success").aggregate().whenComplete((unused, cause) -> { + assertThat(eventLoop.get().inEventLoop()).isTrue(); + latch.countDown(); + }); + ctx = captor.get(); + } latch.await(); + awaitValidClientRequestContext(ctx, 2); } private WebClient client(RetryRule retryRule) { @@ -927,4 +1075,24 @@ public Stream provideArguments(ExtensionContext context) th return Stream.of(retryRule).map(Arguments::of); } } + + private static void awaitValidClientRequestContext(ClientRequestContext ctx) { + awaitValidClientRequestContext(ctx, ctx.log().children().size()); + } + + private static void awaitValidClientRequestContext(ClientRequestContext ctx, int expectedNumRequests) { + await().untilAsserted(() -> { + assertThat(ctx.log().isComplete()).isTrue(); + assertThat(ctx.log().children()).hasSize(expectedNumRequests); + ctx.log().children().forEach(childLogAccess -> { + try { + final RequestLog childLog = childLogAccess.whenComplete().get(); + assertThat(childLog).isNotNull(); + assertThat(childLog.isComplete()).isTrue(); + } catch (InterruptedException | ExecutionException e) { + fail(e); + } + }); + }); + } } From a6b77a4d8a66fd7d6fffbac8c87067e10f13ac38 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 25 Jun 2025 22:46:45 +0200 Subject: [PATCH 02/12] test: add more detailed verification of child logs --- .../client/retry/RetryingClientTest.java | 266 +++++++++++++----- .../internal/testing/RequestContextUtils.java | 179 ++++++++++++ 2 files changed, 378 insertions(+), 67 deletions(-) create mode 100644 testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index 05fd46654df..3e2accdea9c 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -18,10 +18,18 @@ import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; import static com.linecorp.armeria.common.util.Exceptions.peel; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidClientRequestContext; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidClientRequestContextWithParentLogVerifier; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseHeader; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseTrailer; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyStatusCode; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyUnprocessedRequestException; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.catchThrowable; -import static org.assertj.core.api.Assertions.fail; import static org.awaitility.Awaitility.await; import java.time.Duration; @@ -33,7 +41,6 @@ import java.util.concurrent.CompletionStage; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -80,8 +87,10 @@ import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.logging.RequestLog; import com.linecorp.armeria.common.stream.AbortedStreamException; +import com.linecorp.armeria.common.stream.CancelledSubscriptionException; import com.linecorp.armeria.common.util.UnmodifiableFuture; import com.linecorp.armeria.internal.testing.AnticipatedException; +import com.linecorp.armeria.internal.testing.RequestContextUtils.RequestLogVerifier; import com.linecorp.armeria.server.AbstractHttpService; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.ServiceRequestContext; @@ -328,7 +337,7 @@ void setUp() { } @Test - void retryWhenContentMatched() { + void retryContentMatched() { final Function retryingDecorator = RetryingClient.builder(new RetryIfContentMatch("Need to retry"), 1024) .newDecorator(); @@ -342,7 +351,11 @@ void retryWhenContentMatched() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, 3); + awaitValidClientRequestContext(ctx, + verifyStatusCode(HttpStatus.OK), + verifyStatusCode(HttpStatus.OK), + verifyStatusCode(HttpStatus.OK) + ); } @Test @@ -354,7 +367,8 @@ void retryWhenStatusMatched() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -369,7 +383,10 @@ void retryWhenStatusMatchedWithContent() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, 2); + await().untilAsserted( + () -> awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)) + ); } @Test @@ -386,7 +403,16 @@ void retryWhenTrailerMatched() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, + verifyAllValid( + verifyStatusCode(HttpStatus.OK), + verifyResponseTrailer("grpc-status", "3") + ), + verifyAllValid( + verifyStatusCode(HttpStatus.OK), + verifyResponseTrailer("grpc-status", "0") + ) + ); } @Test @@ -401,7 +427,16 @@ void retryWhenTotalDurationIsHigh() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx); + + await().untilAsserted(() -> { + assertThat(ctx.log().isComplete()).isTrue(); + }); + + final int actualAttemptCount = ctx.log().children().size(); + final RequestLogVerifier[] clientLogVerifiers = new RequestLogVerifier[actualAttemptCount]; + Arrays.fill(clientLogVerifiers, verifyStatusCode(HttpStatus.OK)); + clientLogVerifiers[0] = verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE); + awaitValidClientRequestContext(ctx, clientLogVerifiers); } @Test @@ -414,7 +449,8 @@ void disableResponseTimeout() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // response timeout did not happen. - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -429,13 +465,20 @@ void respectRetryAfter() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + assertThat(res.headers().get(HttpHeaderNames.RETRY_AFTER.toString())).isNull(); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo( (long) (TimeUnit.SECONDS.toMillis(1) * 0.9)); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, + verifyAllValid( + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyResponseHeader( + HttpHeaderNames.RETRY_AFTER.toString(), + "1")), + verifyStatusCode(HttpStatus.OK)); } @Test - void respectRetryAfterWithHttpDate() { + void respectRetryAfterWithHttpDate() throws InterruptedException { final WebClient client = client(RetryRule.failsafe()); final Stopwatch sw = Stopwatch.createStarted(); @@ -446,10 +489,22 @@ void respectRetryAfterWithHttpDate() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); + final String expectedRetryAfterHeader = + server.requestContextCaptor().take().log().partial().responseHeaders().get( + HttpHeaderNames.RETRY_AFTER.toString()); + assertThat(expectedRetryAfterHeader).isNotNull(); + + // This header should be transferred to the caller as retrying should be transparent. + assertThat(res.headers().get(HttpHeaderNames.RETRY_AFTER.toString())).isNull(); // Since ZonedDateTime doesn't express exact time, // just check out whether it is retried after delayed some time. assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo(1000); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyAllValid(verifyStatusCode( + HttpStatus.SERVICE_UNAVAILABLE), + verifyResponseHeader( + HttpHeaderNames.RETRY_AFTER.toString(), + expectedRetryAfterHeader)), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -464,7 +519,7 @@ void propagateLastResponseWhenNextRetryIsAfterTimeout() { ctx = captor.get(); } assertThat(res.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); - awaitValidClientRequestContext(ctx, 1); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE)); } @Test @@ -477,11 +532,15 @@ void propagateLastResponseWhenExceedMaxAttempts() { ctx = captor.get(); } assertThat(res.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); - awaitValidClientRequestContext(ctx, 3); // equal to max attempts + // maximum number of attempts + awaitValidClientRequestContext(ctx, + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE)); } @Test - void retryAfterOneYear() { + void retryAfterOneYear() throws InterruptedException { final WebClient client = client(RetryRule.failsafe()); // The response will be the last response whose headers contains HttpHeaderNames.RETRY_AFTER // because next retry is after timeout @@ -492,7 +551,20 @@ void retryAfterOneYear() { } assertThat(headers.status()).isSameAs(HttpStatus.SERVICE_UNAVAILABLE); assertThat(headers.get(HttpHeaderNames.RETRY_AFTER)).isNotNull(); - awaitValidClientRequestContext(ctx, 1); + final String expectedRetryAfterHeader = server + .requestContextCaptor() + .take() + .log() + .partial() + .responseHeaders() + .get(HttpHeaderNames.RETRY_AFTER.toString()); + assertThat(expectedRetryAfterHeader).isNotNull(); + + awaitValidClientRequestContext(ctx, verifyAllValid(verifyStatusCode( + HttpStatus.SERVICE_UNAVAILABLE), + verifyResponseHeader( + HttpHeaderNames.RETRY_AFTER.toString(), + expectedRetryAfterHeader))); } @Test @@ -515,7 +587,10 @@ void retryOnResponseTimeout() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(ResponseTimeoutException.class) + ), verifyStatusCode(HttpStatus.OK)); } @Test @@ -544,7 +619,11 @@ void retryWithContentOnResponseTimeout() { assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // Make sure that all customized RetryRuleWithContents are called. assertThat(queue).containsExactly(1, 2, 3); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(ResponseTimeoutException.class) + ), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -587,7 +666,8 @@ void honorRetryMapping() { ctx = captor.get(); } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(2), Duration.ofSeconds(6)); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.INTERNAL_SERVER_ERROR), + verifyStatusCode(HttpStatus.INTERNAL_SERVER_ERROR)); stopwatch = Stopwatch.createStarted(); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { @@ -596,7 +676,15 @@ void honorRetryMapping() { ctx = captor.get(); } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(14), Duration.ofSeconds(28)); - awaitValidClientRequestContext(ctx, 8); + awaitValidClientRequestContext(ctx, + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED), + verifyStatusCode(HttpStatus.NOT_IMPLEMENTED)); stopwatch = Stopwatch.createStarted(); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { @@ -605,7 +693,7 @@ void honorRetryMapping() { ctx = captor.get(); } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(0), Duration.ofSeconds(2)); - awaitValidClientRequestContext(ctx, 1); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.BAD_GATEWAY)); } @Test @@ -632,7 +720,8 @@ void evaluatesMappingOnce() { // 1 logical request; 2 retries assertThat(evaluations.get()).isEqualTo(1); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.INTERNAL_SERVER_ERROR), + verifyStatusCode(HttpStatus.OK)); reqCount.set(0); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { @@ -643,7 +732,8 @@ void evaluatesMappingOnce() { // 2 logical requests; 2 retries assertThat(evaluations.get()).isEqualTo(2); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.INTERNAL_SERVER_ERROR), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -683,7 +773,12 @@ void retryWithContentOnUnprocessedException() { ctx = captor.get(); } assertThat(stopwatch.elapsed()).isBetween(Duration.ofSeconds(7), Duration.ofSeconds(20)); - awaitValidClientRequestContext(ctx, 5); // max attempts + awaitValidClientRequestContext(ctx, + verifyUnprocessedRequestException(), + verifyUnprocessedRequestException(), + verifyUnprocessedRequestException(), + verifyUnprocessedRequestException(), + verifyUnprocessedRequestException()); } } @@ -700,7 +795,8 @@ void differentBackoffBasedOnStatus(RetryRule retryRule) { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isBetween((long) (10 * 0.9), (long) (1000 * 1.1)); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)); reqCount.set(0); sw.reset().start(); @@ -711,7 +807,8 @@ void differentBackoffBasedOnStatus(RetryRule retryRule) { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo((long) (1000 * 0.9)); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.INTERNAL_SERVER_ERROR), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -726,7 +823,9 @@ void retryWithRequestBody() { ctx = captor.get(); } assertThat(res.contentUtf8()).isEqualTo("bar"); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)); } @Test @@ -769,11 +868,20 @@ void shouldGetExceptionWhenFactoryIsClosed() { t = peel(catchThrowable(() -> client.get("/service-unavailable").aggregate().join())); ctx = captor.get(); } - awaitValidClientRequestContext(ctx, 1); // not able to schedule second retry. if (t instanceof UnprocessedRequestException) { final Throwable cause = t.getCause(); assertThat(cause).isInstanceOf(IllegalStateException.class); t = cause; + // not able to schedule second retry. + awaitValidClientRequestContext(ctx, verifyUnprocessedRequestException()); + } else { + awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(IllegalStateException.class) + ) + )); } assertThat(t).isInstanceOf(IllegalStateException.class) .satisfies(cause -> assertThat(cause.getMessage()).matches( @@ -785,14 +893,9 @@ void doNotRetryWhenResponseIsAborted() throws Exception { final List abortCauses = Arrays.asList(null, new IllegalStateException("abort stream with a specified cause")); for (Throwable abortCause : abortCauses) { - final AtomicReference context = new AtomicReference<>(); final WebClient client = WebClient.builder(server.httpUri()) .decorator(RetryingClient.newDecorator(retryAlways)) - .decorator((delegate, ctx, req) -> { - context.set(ctx); - return delegate.execute(ctx, req); - }) .decorator(LoggingClient.newDecorator()) .build(); responseAbortServiceCallCounter.set(0); @@ -806,21 +909,28 @@ void doNotRetryWhenResponseIsAborted() throws Exception { } else { httpResponse.abort(abortCause); } - awaitValidClientRequestContext(ctx, 1); - final RequestLog log = context.get().log().whenComplete().join(); + final RequestLog log = ctx.log().whenComplete().join(); final Throwable requestCause = log.requestCause(); if (abortCause == null) { assertThat(log.responseCause()).isExactlyInstanceOf(AbortedStreamException.class); + awaitValidClientRequestContext(ctx, verifyResponseCause(AbortedStreamException.class)); if (requestCause != null) { // A request can either successfully complete or fail depending on timing. assertThat(requestCause).isExactlyInstanceOf(AbortedStreamException.class); } } else { - assertThat(log.responseCause()).isSameAs(abortCause); if (requestCause != null) { + awaitValidClientRequestContext(ctx, + verifyExactlyOneValid( + verifyResponseCause(abortCause), + verifyResponseCause(AbortedStreamException.class) + ) + ); // A request can either successfully complete or fail depending on timing. assertThat(requestCause).isSameAs(abortCause); + } else { + awaitValidClientRequestContext(ctx, verifyResponseCause(requestCause)); } } @@ -855,9 +965,19 @@ public void onComplete() {} ctx = captor.get(); } - awaitValidClientRequestContext(ctx, 1); + TimeUnit.SECONDS.sleep(1L); // Sleep to check if there's a retry. assertThat(subscriberCancelServiceCallCounter.get()).isEqualTo(1); + awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + verifyAllValid( + verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyResponseCause(AbortedStreamException.class) + ), + verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(CancelledSubscriptionException.class) + ) + )); } @Test @@ -890,7 +1010,7 @@ void doNotRetryWhenRequestIsAborted() throws Exception { TimeUnit.SECONDS.sleep(1); // No request is made. assertThat(responseAbortServiceCallCounter.get()).isZero(); - awaitValidClientRequestContext(ctx, 0); + awaitValidClientRequestContext(ctx); final RequestLog log = context.get().log().whenComplete().join(); if (abortCause == null) { assertThat(log.requestCause()).isExactlyInstanceOf(AbortedStreamException.class); @@ -923,7 +1043,12 @@ void exceptionInDecorator() { ctx = captor.get(); } assertThat(retryCounter.get()).isEqualTo(5); - awaitValidClientRequestContext(ctx, 5); + // max attempts + awaitValidClientRequestContext(ctx, verifyResponseCause(AnticipatedException.class), + verifyResponseCause(AnticipatedException.class), + verifyResponseCause(AnticipatedException.class), + verifyResponseCause(AnticipatedException.class), + verifyResponseCause(AnticipatedException.class)); } @Test @@ -940,7 +1065,10 @@ void exceptionInRule() { .hasCauseReference(exception); ctx = captor.get(); } - awaitValidClientRequestContext(ctx, 1); + awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + verifyResponseCause(AbortedStreamException.class), + verifyResponseCause(exception) + )); } @Test @@ -950,14 +1078,23 @@ void exceptionInRuleWithContent() { throw exception; }; - final WebClient client = client(rule, 10000, 0, 100); + final WebClient client = client(rule, + 10000, 0, 100); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { assertThatThrownBy(client.get("/").aggregate()::join) .isInstanceOf(CompletionException.class) .hasCauseReference(exception); ctx = captor.get(); } - awaitValidClientRequestContext(ctx, 1); + + await().untilAsserted(() -> { + ctx.log().isComplete(); + }); + assertThat(ctx.log().children()).hasSize(1); + + awaitValidClientRequestContextWithParentLogVerifier(ctx, + verifyStatusCode(HttpStatus.UNKNOWN), + verifyStatusCode(HttpStatus.NOT_FOUND)); } @Test @@ -973,14 +1110,16 @@ void useSameEventLoopWhenAggregate() throws InterruptedException { .decorator(RetryingClient.newDecorator(RetryRule.failsafe(), 2)) .build(); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { - client.get("/503-then-success").aggregate().whenComplete((unused, cause) -> { - assertThat(eventLoop.get().inEventLoop()).isTrue(); - latch.countDown(); - }); + client.get("/503-then-success").aggregate() + .whenComplete((unused, cause) -> { + assertThat(eventLoop.get().inEventLoop()).isTrue(); + latch.countDown(); + }); ctx = captor.get(); } latch.await(); - awaitValidClientRequestContext(ctx, 2); + awaitValidClientRequestContext(ctx, verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), + verifyStatusCode(HttpStatus.OK)); } private WebClient client(RetryRule retryRule) { @@ -1034,6 +1173,19 @@ private WebClient client(RetryRuleWithContent retryRuleWithContent .build(); } + private static void awaitValidClientRequestContext(ClientRequestContext ctx, + RequestLogVerifier... childLogVerifiers) { + await().untilAsserted(() -> assertValidClientRequestContext(ctx, childLogVerifiers)); + } + + public static void awaitValidClientRequestContextWithParentLogVerifier( + ClientRequestContext ctx, + RequestLogVerifier parentLogVerifier, + RequestLogVerifier... childLogVerifiers) { + await().untilAsserted(() -> assertValidClientRequestContextWithParentLogVerifier(ctx, parentLogVerifier, + childLogVerifiers)); + } + private static class RetryIfContentMatch implements RetryRuleWithContent { private final String retryContent; private final RetryDecision decision = RetryDecision.retry(Backoff.fixed(100)); @@ -1075,24 +1227,4 @@ public Stream provideArguments(ExtensionContext context) th return Stream.of(retryRule).map(Arguments::of); } } - - private static void awaitValidClientRequestContext(ClientRequestContext ctx) { - awaitValidClientRequestContext(ctx, ctx.log().children().size()); - } - - private static void awaitValidClientRequestContext(ClientRequestContext ctx, int expectedNumRequests) { - await().untilAsserted(() -> { - assertThat(ctx.log().isComplete()).isTrue(); - assertThat(ctx.log().children()).hasSize(expectedNumRequests); - ctx.log().children().forEach(childLogAccess -> { - try { - final RequestLog childLog = childLogAccess.whenComplete().get(); - assertThat(childLog).isNotNull(); - assertThat(childLog.isComplete()).isTrue(); - } catch (InterruptedException | ExecutionException e) { - fail(e); - } - }); - }); - } } diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java new file mode 100644 index 00000000000..398eb3ac644 --- /dev/null +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -0,0 +1,179 @@ +/* + * Copyright 2025 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.armeria.internal.testing; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; +import static org.assertj.core.api.Assertions.fail; + +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; + +import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.client.UnprocessedRequestException; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.ResponseHeaders; +import com.linecorp.armeria.common.logging.RequestLog; + +public final class RequestContextUtils { + private RequestContextUtils() {} + + @FunctionalInterface + public interface RequestLogVerifier { + void verifyChildLog(RequestLog childLog) throws Exception; + } + + public static final RequestLogVerifier VERIFY_NOTHING = childLog -> { + // No verification is performed. + }; + + public static RequestLogVerifier verifyAllValid(RequestLogVerifier... childLogVerifiers) { + return childLog -> { + for (RequestLogVerifier childLogVerifier : childLogVerifiers) { + childLogVerifier.verifyChildLog(childLog); + } + }; + } + + public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... childLogVerifiers) { + return childLog -> { + final Throwable[] verifierCauses = new Throwable[childLogVerifiers.length]; + + for (int i = 0; i < childLogVerifiers.length; i++) { + final int index = i; + verifierCauses[i] = catchThrowable(() -> childLogVerifiers[index].verifyChildLog(childLog)); + } + + final List nonNullVerifierCauses = Arrays.stream(verifierCauses) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + + if (nonNullVerifierCauses.size() != childLogVerifiers.length - 1) { + final Throwable allCauses = nonNullVerifierCauses.get(0); + + for (int i = 1; i < nonNullVerifierCauses.size(); i++) { + allCauses.addSuppressed(nonNullVerifierCauses.get(i)); + } + + fail(allCauses); + } + }; + } + + public static RequestLogVerifier verifyStatusCode(HttpStatus expectedStatus) { + return childLog -> assertThat(childLog.responseHeaders().status()).isEqualTo(expectedStatus); + } + + public static RequestLogVerifier verifyUnprocessedRequestException() { + return verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(UnprocessedRequestException.class) + ); + } + + public static RequestLogVerifier verifyResponseCause(Class expectedResponseCauseClass) { + return childLog -> { + assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedResponseCauseClass); + }; + } + + public static RequestLogVerifier verifyResponseCause(Throwable expectedCause) { + return childLog -> { + assertThat(childLog.responseCause()).isSameAs(expectedCause); + }; + } + + public static RequestLogVerifier verifyResponseHeader(String headerName, + String expectedHeaderValue) { + return childLog -> { + final ResponseHeaders headers = childLog.responseHeaders(); + assertThat(headers.get(headerName)).isEqualTo(expectedHeaderValue); + }; + } + + public static RequestLogVerifier verifyResponseTrailer(String headerName, + String expectedHeaderValue) { + return childLog -> { + assertThat(childLog.responseTrailers().get(headerName)).isEqualTo(expectedHeaderValue); + }; + } + + public static void assertValidClientRequestContext(ClientRequestContext ctx, + RequestLogVerifier... childLogVerifiers) { + assertValidClientRequestContextWithVerifier(ctx, childLogVerifiers); + } + + public static void assertValidClientRequestContextWithParentLogVerifier( + ClientRequestContext ctx, + RequestLogVerifier parentLogVerifier, + RequestLogVerifier... childLogVerifiers) { + assertValidClientRequestContextWithVerifier(ctx, parentLogVerifier, childLogVerifiers); + } + + private static void assertValidClientRequestContextWithVerifier( + ClientRequestContext ctx, + RequestLogVerifier[] childLogVerifiers) { + if (childLogVerifiers.length == 0) { + childLogVerifiers = new RequestLogVerifier[ctx.log().children().size()]; + Arrays.fill(childLogVerifiers, VERIFY_NOTHING); + } + + assertValidClientRequestContextWithVerifier( + ctx, + childLogVerifiers.length == 0 ? + VERIFY_NOTHING : childLogVerifiers[childLogVerifiers.length - 1], childLogVerifiers + ); + } + + private static void assertValidClientRequestContextWithVerifier( + ClientRequestContext ctx, + RequestLogVerifier parentLogVerifier, + RequestLogVerifier[] childLogVerifiers) { + final int expectedNumRequests = childLogVerifiers.length; + + assertThat(ctx.log().isComplete()).isTrue(); + assertThat(ctx.log().children()).hasSize(expectedNumRequests); + + if (expectedNumRequests == 0) { + return; + } + + for (int childLogIndex = 0; childLogIndex < expectedNumRequests; childLogIndex++) { + final RequestLog childLog = ctx.log().children().get(childLogIndex).whenComplete().join(); + assertThat(childLog).isNotNull(); + assertThat(childLog.isComplete()).isTrue(); + assertThat(childLog.children()).isEmpty(); + assertThat(childLog.requestContent()).isNull(); + assertThat(childLog.responseContent()).isNull(); + assertThat(childLog.rawResponseContent()).isNull(); + assertThat(childLog.responseContentPreview()).isNull(); + try { + childLogVerifiers[childLogIndex].verifyChildLog(childLog); + } catch (Throwable e) { + fail("Failed to verify child log (" + (childLogIndex + 1) + + '/' + expectedNumRequests + ')', e); + } + } + try { + parentLogVerifier.verifyChildLog(ctx.log().partial()); + } catch (Throwable e) { + fail("Failed to verify parent log", e); + } + } +} From fd2f76829d6167f6267de981f2ce6ccc42c381cf Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Thu, 26 Jun 2025 11:56:23 +0200 Subject: [PATCH 03/12] test: add client request context verifications to `RetryingRpcClientTest` --- .../client/retry/RetryingClientTest.java | 8 +- .../internal/testing/RequestContextUtils.java | 119 +++++++---- .../client/retry/RetryingRpcClientTest.java | 195 +++++++++++++++--- 3 files changed, 252 insertions(+), 70 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index 3e2accdea9c..3d18bbb97ab 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -18,8 +18,8 @@ import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; import static com.linecorp.armeria.common.util.Exceptions.peel; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidClientRequestContext; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidClientRequestContextWithParentLogVerifier; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; @@ -1175,14 +1175,14 @@ private WebClient client(RetryRuleWithContent retryRuleWithContent private static void awaitValidClientRequestContext(ClientRequestContext ctx, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidClientRequestContext(ctx, childLogVerifiers)); + await().untilAsserted(() -> assertValidRequestContext(ctx, childLogVerifiers)); } public static void awaitValidClientRequestContextWithParentLogVerifier( ClientRequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidClientRequestContextWithParentLogVerifier(ctx, parentLogVerifier, + await().untilAsserted(() -> assertValidRequestContextWithParentLogVerifier(ctx, parentLogVerifier, childLogVerifiers)); } diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index 398eb3ac644..e04bfb4f313 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -25,10 +25,13 @@ import java.util.Objects; import java.util.stream.Collectors; -import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.UnprocessedRequestException; +import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.ResponseHeaders; +import com.linecorp.armeria.common.RpcRequest; +import com.linecorp.armeria.common.RpcResponse; import com.linecorp.armeria.common.logging.RequestLog; public final class RequestContextUtils { @@ -87,9 +90,21 @@ public static RequestLogVerifier verifyUnprocessedRequestException() { ); } - public static RequestLogVerifier verifyResponseCause(Class expectedResponseCauseClass) { + public static RequestLogVerifier verifyRequestCause(Class expectedCauseClass) { return childLog -> { - assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedResponseCauseClass); + assertThat(childLog.requestCause()).isExactlyInstanceOf(expectedCauseClass); + }; + } + + public static RequestLogVerifier verifyRequestCause(Throwable expectedCause) { + return childLog -> { + assertThat(childLog.requestCause()).isSameAs(expectedCause); + }; + } + + public static RequestLogVerifier verifyResponseCause(Class expectedCauseClass) { + return childLog -> { + assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedCauseClass); }; } @@ -100,7 +115,7 @@ public static RequestLogVerifier verifyResponseCause(Throwable expectedCause) { } public static RequestLogVerifier verifyResponseHeader(String headerName, - String expectedHeaderValue) { + String expectedHeaderValue) { return childLog -> { final ResponseHeaders headers = childLog.responseHeaders(); assertThat(headers.get(headerName)).isEqualTo(expectedHeaderValue); @@ -114,66 +129,94 @@ public static RequestLogVerifier verifyResponseTrailer(String headerName, }; } - public static void assertValidClientRequestContext(ClientRequestContext ctx, - RequestLogVerifier... childLogVerifiers) { - assertValidClientRequestContextWithVerifier(ctx, childLogVerifiers); + public static RequestLogVerifier verifyResponseContent(String expectedResponseContent) { + return childLog -> { + assertThat(childLog.responseContent()).isExactlyInstanceOf(String.class); + assertThat(childLog.responseContent()).isEqualTo(expectedResponseContent); + }; + } + + public static void assertValidRequestContext(RequestContext ctx, + RequestLogVerifier... childLogVerifiers) { + assertValidRequestContextWithVerifier(ctx, childLogVerifiers); } - public static void assertValidClientRequestContextWithParentLogVerifier( - ClientRequestContext ctx, + public static void assertValidRequestContextWithParentLogVerifier( + RequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { - assertValidClientRequestContextWithVerifier(ctx, parentLogVerifier, childLogVerifiers); + assertValidRequestContextWithVerifier(ctx, parentLogVerifier, childLogVerifiers); } - private static void assertValidClientRequestContextWithVerifier( - ClientRequestContext ctx, + private static void assertValidRequestContextWithVerifier( + RequestContext ctx, RequestLogVerifier[] childLogVerifiers) { if (childLogVerifiers.length == 0) { childLogVerifiers = new RequestLogVerifier[ctx.log().children().size()]; Arrays.fill(childLogVerifiers, VERIFY_NOTHING); } - assertValidClientRequestContextWithVerifier( + assertValidRequestContextWithVerifier( ctx, childLogVerifiers.length == 0 ? - VERIFY_NOTHING : childLogVerifiers[childLogVerifiers.length - 1], childLogVerifiers + VERIFY_NOTHING + : verifyAllValid( + childLog -> { + // Default parent log verifier. + final HttpRequest req = ctx.request(); + assertThat(req).isNotNull(); + assert req != null; + assertThat(req.isComplete()).isTrue(); + + if (ctx.rpcRequest() != null) { + final HttpRequest lastHttpReq = + ctx.log().children() + .get(ctx.log().children().size() - 1).context().request(); + + if (lastHttpReq != null) { + assertThat(lastHttpReq).isSameAs(ctx.log().context().request()); + } + } + }, + childLogVerifiers[childLogVerifiers.length - 1] + ), childLogVerifiers ); } - private static void assertValidClientRequestContextWithVerifier( - ClientRequestContext ctx, + private static void assertValidRequestContextWithVerifier( + RequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier[] childLogVerifiers) { final int expectedNumRequests = childLogVerifiers.length; + assertThat(ctx.log().isComplete()).isTrue(); + assertThat(ctx.log().children()).hasSize(expectedNumRequests); - assertThat(ctx.log().isComplete()).isTrue(); - assertThat(ctx.log().children()).hasSize(expectedNumRequests); + if (expectedNumRequests == 0) { + return; + } - if (expectedNumRequests == 0) { - return; + for (int childLogIndex = 0; childLogIndex < expectedNumRequests; childLogIndex++) { + final RequestLog childLog = ctx.log().children().get(childLogIndex).whenComplete().join(); + assertThat(childLog).isNotNull(); + assertThat(childLog.isComplete()).isTrue(); + assertThat(childLog.children()).isEmpty(); + if (ctx.rpcRequest() != null) { + assertThat(childLog.requestContent()).isInstanceOf(RpcRequest.class); + assertThat(childLog.responseContent()).isInstanceOf(RpcResponse.class); } - for (int childLogIndex = 0; childLogIndex < expectedNumRequests; childLogIndex++) { - final RequestLog childLog = ctx.log().children().get(childLogIndex).whenComplete().join(); - assertThat(childLog).isNotNull(); - assertThat(childLog.isComplete()).isTrue(); - assertThat(childLog.children()).isEmpty(); - assertThat(childLog.requestContent()).isNull(); - assertThat(childLog.responseContent()).isNull(); - assertThat(childLog.rawResponseContent()).isNull(); - assertThat(childLog.responseContentPreview()).isNull(); - try { - childLogVerifiers[childLogIndex].verifyChildLog(childLog); - } catch (Throwable e) { - fail("Failed to verify child log (" + (childLogIndex + 1) + - '/' + expectedNumRequests + ')', e); - } - } try { - parentLogVerifier.verifyChildLog(ctx.log().partial()); + childLogVerifiers[childLogIndex].verifyChildLog(childLog); } catch (Throwable e) { - fail("Failed to verify parent log", e); + fail("Failed to verify child log (" + (childLogIndex + 1) + + '/' + expectedNumRequests + ')', e); } + } + + try { + parentLogVerifier.verifyChildLog(ctx.log().partial()); + } catch (Throwable e) { + fail("Failed to verify parent log", e); + } } } diff --git a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java index 1e56c8b0472..b2b97b45be1 100644 --- a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java +++ b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java @@ -16,6 +16,12 @@ package com.linecorp.armeria.it.client.retry; import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyRequestCause; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyStatusCode; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.catchThrowable; @@ -31,7 +37,6 @@ import java.util.concurrent.BlockingQueue; import java.util.concurrent.CancellationException; import java.util.concurrent.Executors; -import java.util.concurrent.LinkedTransferQueue; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -42,6 +47,8 @@ import com.linecorp.armeria.client.ClientFactory; import com.linecorp.armeria.client.ClientRequestContext; +import com.linecorp.armeria.client.ClientRequestContextCaptor; +import com.linecorp.armeria.client.Clients; import com.linecorp.armeria.client.UnprocessedRequestException; import com.linecorp.armeria.client.retry.Backoff; import com.linecorp.armeria.client.retry.RetryConfig; @@ -50,10 +57,13 @@ import com.linecorp.armeria.client.retry.RetryRuleWithContent; import com.linecorp.armeria.client.retry.RetryingRpcClient; import com.linecorp.armeria.client.thrift.ThriftClients; -import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.CompletableRpcResponse; +import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.RpcResponse; +import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.logging.RequestLog; import com.linecorp.armeria.common.util.UnmodifiableFuture; +import com.linecorp.armeria.internal.testing.RequestContextUtils.RequestLogVerifier; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.thrift.THttpService; import com.linecorp.armeria.testing.junit5.server.ServerExtension; @@ -74,6 +84,8 @@ class RetryingRpcClientTest { private final DevNullService.Iface devNullServiceHandler = mock(DevNullService.Iface.class); private final AtomicInteger serviceRetryCount = new AtomicInteger(); + private ClientRequestContext ctx; + @RegisterExtension final ServerExtension server = new ServerExtension() { @Override @@ -99,10 +111,15 @@ protected void configure(ServerBuilder sb) throws Exception { @Test void execute() throws Exception { final HelloService.Iface client = helloClient(retryOnException, 100); + when(serviceHandler.hello(anyString())).thenReturn("world"); - assertThat(client.hello("hello")).isEqualTo("world"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.hello("hello")).isEqualTo("world"); + ctx = captor.get(); + } verify(serviceHandler, only()).hello("hello"); + awaitValidClientRequestContext(ctx, verifyResponse("world")); } @Test @@ -127,8 +144,15 @@ void execute_honorMapping() throws Exception { .thenThrow(new IllegalArgumentException()) .thenReturn("Hey"); serviceRetryCount.set(0); - assertThat(client.hello("Alice")).isEqualTo("Hey"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.hello("Alice")).isEqualTo("Hey"); + ctx = captor.get(); + } verify(serviceHandler, times(3)).hello("Alice"); + awaitValidClientRequestContext(ctx, + verifyResponseException(), + verifyResponseException(), + verifyResponse("Hey")); when(serviceHandler.hello(anyString())) .thenThrow(new IllegalArgumentException()) @@ -139,6 +163,10 @@ void execute_honorMapping() throws Exception { serviceRetryCount.set(0); assertThat(client.hello("Bob")).isEqualTo("Hey"); verify(serviceHandler, times(5)).hello("Bob"); + awaitValidClientRequestContext(ctx, + verifyResponseException(), + verifyResponseException(), + verifyResponse("Hey")); } @Test @@ -159,10 +187,15 @@ void evaluatesMappingOnce() throws Exception { .thenThrow(new IllegalArgumentException()) .thenReturn("Hey"); - assertThat(client.hello("Alice")).isEqualTo("Hey"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.hello("Alice")).isEqualTo("Hey"); + ctx = captor.get(); + } // 1 logical request; 3 retries assertThat(evaluations.get()).isEqualTo(1); verify(serviceHandler, times(3)).hello("Alice"); + awaitValidClientRequestContext(ctx, verifyResponseException(), verifyResponseException(), + verifyResponse("Hey")); serviceRetryCount.set(0); @@ -171,10 +204,15 @@ void evaluatesMappingOnce() throws Exception { .thenThrow(new IllegalArgumentException()) .thenReturn("Hey"); - assertThat(client.hello("Alice")).isEqualTo("Hey"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.hello("Alice")).isEqualTo("Hey"); + ctx = captor.get(); + } // 2 logical requests total; 6 retries total assertThat(evaluations.get()).isEqualTo(2); verify(serviceHandler, times(6)).hello("Alice"); + awaitValidClientRequestContext(ctx, verifyResponseException(), verifyResponseException(), + verifyResponse("Hey")); } @Test @@ -185,8 +223,13 @@ void execute_retry() throws Exception { .thenThrow(new IllegalArgumentException()) .thenReturn("world"); - assertThat(client.hello("hello")).isEqualTo("world"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThat(client.hello("hello")).isEqualTo("world"); + ctx = captor.get(); + } verify(serviceHandler, times(3)).hello("hello"); + awaitValidClientRequestContext(ctx, verifyResponseException(), + verifyResponseException(), verifyResponse("world")); } @Test @@ -194,31 +237,33 @@ void execute_reachedMaxAttempts() throws Exception { final HelloService.Iface client = helloClient(retryAlways, 2); when(serviceHandler.hello(anyString())).thenThrow(new IllegalArgumentException()); - final Throwable thrown = catchThrowable(() -> client.hello("hello")); + final Throwable thrown; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + thrown = catchThrowable(() -> client.hello("hello")); + ctx = captor.get(); + } assertThat(thrown).isInstanceOf(TApplicationException.class); assertThat(((TApplicationException) thrown).getType()).isEqualTo(TApplicationException.INTERNAL_ERROR); verify(serviceHandler, times(2)).hello("hello"); + awaitValidClientRequestContext(ctx, verifyResponseException(), verifyResponseException()); } @Test void propagateLastResponseWhenNextRetryIsAfterTimeout() throws Exception { - final BlockingQueue logQueue = new LinkedTransferQueue<>(); final RetryRuleWithContent rule = (ctx, response, cause) -> UnmodifiableFuture.completedFuture( RetryDecision.retry(Backoff.fixed(10000000))); - final HelloService.Iface client = helloClient(rule, 100, logQueue); + final HelloService.Iface client = helloClient(rule, 100); when(serviceHandler.hello(anyString())).thenThrow(new IllegalArgumentException()); - final Throwable thrown = catchThrowable(() -> client.hello("hello")); + final Throwable thrown; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + thrown = catchThrowable(() -> client.hello("hello")); + ctx = captor.get(); + } assertThat(thrown).isInstanceOf(TApplicationException.class); assertThat(((TApplicationException) thrown).getType()).isEqualTo(TApplicationException.INTERNAL_ERROR); verify(serviceHandler, only()).hello("hello"); - - // Make sure the last HTTP request is set to the parent's HTTP request. - final RequestLog log = logQueue.poll(10, TimeUnit.SECONDS); - assertThat(log).isNotNull(); - assertThat(log.children()).isNotEmpty(); - final HttpRequest lastHttpReq = log.children().get(log.children().size() - 1).context().request(); - assertThat(lastHttpReq).isSameAs(log.context().request()); + awaitValidClientRequestContext(ctx, verifyResponseException()); } @Test @@ -228,7 +273,20 @@ void exceptionInStrategy() { throw exception; }, Integer.MAX_VALUE); - assertThatThrownBy(() -> client.hello("bar")).isSameAs(exception); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(() -> client.hello("bar")).isSameAs(exception); + ctx = captor.get(); + } + + awaitValidRequestContextWithParentLogVerifier(ctx, + verifyAllValid( + // Not a response exception from the server so + // we are not using verifyResponseCause + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(exception) + ), + verifyResponseException( + TApplicationException.MISSING_RESULT)); } private HelloService.Iface helloClient(RetryConfigMapping mapping) { @@ -277,8 +335,14 @@ void execute_void() throws Exception { .doThrow(new IllegalArgumentException()) .doNothing() .when(devNullServiceHandler).consume(anyString()); - client.consume("hello"); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + client.consume("hello"); + ctx = captor.get(); + } verify(devNullServiceHandler, times(3)).consume("hello"); + awaitValidClientRequestContext(ctx, verifyResponseException(), + verifyResponseException(), verifyResponse(null) + ); } @Test @@ -315,11 +379,33 @@ void shouldGetExceptionWhenFactoryIsClosed() throws Exception { // 3 - In HttpClientDelegate, addressResolverGroup.getResolver(eventLoop) can raise // IllegalStateException("executor not accepting a task"). // - Throwable t = catchThrowable(() -> client.hello("hello")); + Throwable t; + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + t = catchThrowable(() -> client.hello("hello")); + ctx = captor.get(); + } if (t instanceof UnprocessedRequestException) { final Throwable cause = t.getCause(); assertThat(cause).isInstanceOf(IllegalStateException.class); t = cause; + awaitValidClientRequestContext(ctx, verifyAllValid( + // We cannot be sure that we set + // the request cause so we are not checking + // with verifyRequestException/ + // verifyRequestCause(). + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(t) + )); + } else { + awaitValidRequestContextWithParentLogVerifier(ctx, + verifyAllValid( + // Same as above. + verifyStatusCode(HttpStatus.UNKNOWN), + verifyResponseCause(t) + ), + verifyResponseException( + TApplicationException.INTERNAL_ERROR) + ); } assertThat(t).isInstanceOf(IllegalStateException.class) .satisfies(cause -> assertThat(cause.getMessage()).matches( @@ -342,19 +428,72 @@ void doNotRetryWhenResponseIsCancelled() throws Exception { .build(HelloService.Iface.class); when(serviceHandler.hello(anyString())).thenThrow(new IllegalArgumentException()); - assertThatThrownBy(() -> client.hello("hello")).isInstanceOf(CancellationException.class); + try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { + assertThatThrownBy(() -> client.hello("hello")).isInstanceOf(CancellationException.class); + ctx = captor.get(); + } await().untilAsserted(() -> { verify(serviceHandler, only()).hello("hello"); }); - final RequestLog log = context.get().log().whenComplete().join(); - - // ClientUtil.completeLogIfIncomplete() records exceptions caused by response cancellations. - assertThat(log.requestCause()).isExactlyInstanceOf(CancellationException.class); - assertThat(log.responseCause()).isExactlyInstanceOf(CancellationException.class); - // Sleep 1 second more to check if there was another retry. TimeUnit.SECONDS.sleep(1); verify(serviceHandler, only()).hello("hello"); + assertValidRequestContextWithParentLogVerifier( + ctx, + // ClientUtil.completeLogIfIncomplete() records exceptions caused by response cancellations. + verifyRequestException(CancellationException.class), + verifyResponseException(TApplicationException.INTERNAL_ERROR)); + } + + private static void awaitValidClientRequestContext(ClientRequestContext ctx, + RequestLogVerifier... childLogVerifiers) { + await().untilAsserted(() -> assertValidRequestContext(ctx, childLogVerifiers)); + } + + private static void awaitValidRequestContextWithParentLogVerifier(ClientRequestContext ctx, + RequestLogVerifier parentLogVerifier, + RequestLogVerifier... childLogVerifiers) { + await().untilAsserted(() -> assertValidRequestContextWithParentLogVerifier( + ctx, parentLogVerifier, childLogVerifiers)); + } + + private static RequestLogVerifier verifyRequestException(Class causeClass) { + return verifyAllValid( + verifyStatusCode(HttpStatus.UNKNOWN), + verifyRequestCause(causeClass), + verifyResponseCause(causeClass) + ); + } + + private static RequestLogVerifier verifyResponseException() { + return verifyResponseException(TApplicationException.INTERNAL_ERROR); + } + + private static RequestLogVerifier verifyResponseException(int type) { + return verifyAllValid( + verifyStatusCode(HttpStatus.OK), + verifyResponseCause(TApplicationException.class), + childLog -> { + final TApplicationException responseCause = + (TApplicationException) childLog.responseCause(); + + assertThat(responseCause.getType()).isEqualTo(type); + } + ); + } + + private static RequestLogVerifier verifyResponse(@Nullable String expectedResponse) { + return verifyAllValid( + verifyStatusCode(HttpStatus.OK), + childLog -> { + assertThat(childLog.responseContent()).isExactlyInstanceOf(CompletableRpcResponse.class); + final CompletableRpcResponse rpcResponse = + (CompletableRpcResponse) childLog.responseContent(); + assertThat(rpcResponse.isDone()).isTrue(); + assertThat(rpcResponse.isCompletedExceptionally()).isFalse(); + assertThat(rpcResponse.getNow("should-not-be-returned")).isEqualTo(expectedResponse); + } + ); } } From 7c17e7b41e703d3146259cd55a934230ef7bcc91 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Thu, 26 Jun 2025 12:17:11 +0200 Subject: [PATCH 04/12] refactor: remove unused logQueue from `RetryingRpcClientTest` --- .../it/client/retry/RetryingRpcClientTest.java | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java index b2b97b45be1..1af1b80793b 100644 --- a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java +++ b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java @@ -34,7 +34,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.concurrent.BlockingQueue; import java.util.concurrent.CancellationException; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; @@ -61,7 +60,6 @@ import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.RpcResponse; import com.linecorp.armeria.common.annotation.Nullable; -import com.linecorp.armeria.common.logging.RequestLog; import com.linecorp.armeria.common.util.UnmodifiableFuture; import com.linecorp.armeria.internal.testing.RequestContextUtils.RequestLogVerifier; import com.linecorp.armeria.server.ServerBuilder; @@ -307,20 +305,6 @@ private HelloService.Iface helloClient(RetryRuleWithContent rule, i .build(HelloService.Iface.class); } - private HelloService.Iface helloClient(RetryRuleWithContent rule, int maxAttempts, - BlockingQueue logQueue) { - return ThriftClients.builder(server.httpUri()) - .path("/thrift") - .rpcDecorator(RetryingRpcClient.builder(rule) - .maxTotalAttempts(maxAttempts) - .newDecorator()) - .rpcDecorator((delegate, ctx, req) -> { - ctx.log().whenComplete().thenAccept(logQueue::add); - return delegate.execute(ctx, req); - }) - .build(HelloService.Iface.class); - } - @Test void execute_void() throws Exception { final DevNullService.Iface client = From 2834c62773108b01918f6aca62535fdd0400ecac Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Thu, 26 Jun 2025 12:18:04 +0200 Subject: [PATCH 05/12] refactor: remove unused verifyRequestCause(Throwable) from `RequestContextUtils` --- .../armeria/internal/testing/RequestContextUtils.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index e04bfb4f313..f1a1a2b025d 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -96,12 +96,6 @@ public static RequestLogVerifier verifyRequestCause(Class expectedCauseClass) }; } - public static RequestLogVerifier verifyRequestCause(Throwable expectedCause) { - return childLog -> { - assertThat(childLog.requestCause()).isSameAs(expectedCause); - }; - } - public static RequestLogVerifier verifyResponseCause(Class expectedCauseClass) { return childLog -> { assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedCauseClass); From 243f0d824abf858b30c7b2d3f2feae5700d85780 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 12:39:57 +0200 Subject: [PATCH 06/12] refactor: rename `verifyChildLog` to `verifyLog` --- .../armeria/internal/testing/RequestContextUtils.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index f1a1a2b025d..a63b79cfa72 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -39,7 +39,7 @@ private RequestContextUtils() {} @FunctionalInterface public interface RequestLogVerifier { - void verifyChildLog(RequestLog childLog) throws Exception; + void verifyLog(RequestLog requestLog) throws Exception; } public static final RequestLogVerifier VERIFY_NOTHING = childLog -> { @@ -49,7 +49,7 @@ public interface RequestLogVerifier { public static RequestLogVerifier verifyAllValid(RequestLogVerifier... childLogVerifiers) { return childLog -> { for (RequestLogVerifier childLogVerifier : childLogVerifiers) { - childLogVerifier.verifyChildLog(childLog); + childLogVerifier.verifyLog(childLog); } }; } @@ -60,7 +60,7 @@ public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... chi for (int i = 0; i < childLogVerifiers.length; i++) { final int index = i; - verifierCauses[i] = catchThrowable(() -> childLogVerifiers[index].verifyChildLog(childLog)); + verifierCauses[i] = catchThrowable(() -> childLogVerifiers[index].verifyLog(childLog)); } final List nonNullVerifierCauses = Arrays.stream(verifierCauses) @@ -200,7 +200,7 @@ private static void assertValidRequestContextWithVerifier( } try { - childLogVerifiers[childLogIndex].verifyChildLog(childLog); + childLogVerifiers[childLogIndex].verifyLog(childLog); } catch (Throwable e) { fail("Failed to verify child log (" + (childLogIndex + 1) + '/' + expectedNumRequests + ')', e); @@ -208,7 +208,7 @@ private static void assertValidRequestContextWithVerifier( } try { - parentLogVerifier.verifyChildLog(ctx.log().partial()); + parentLogVerifier.verifyLog(ctx.log().partial()); } catch (Throwable e) { fail("Failed to verify parent log", e); } From d9a400a74cf761a6c83d07d77b27ab5d45a0d148 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 12:42:04 +0200 Subject: [PATCH 07/12] refactor: rename `verifyExactlyOneValid` to `verifyExactlyOneVerifierValid` --- .../armeria/client/retry/RetryingClientTest.java | 10 +++++----- .../armeria/internal/testing/RequestContextUtils.java | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index 3d18bbb97ab..c8b7de906e7 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -21,7 +21,7 @@ import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneVerifierValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseHeader; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseTrailer; @@ -875,7 +875,7 @@ void shouldGetExceptionWhenFactoryIsClosed() { // not able to schedule second retry. awaitValidClientRequestContext(ctx, verifyUnprocessedRequestException()); } else { - awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + awaitValidClientRequestContext(ctx, verifyExactlyOneVerifierValid( verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), verifyAllValid( verifyStatusCode(HttpStatus.UNKNOWN), @@ -922,7 +922,7 @@ void doNotRetryWhenResponseIsAborted() throws Exception { } else { if (requestCause != null) { awaitValidClientRequestContext(ctx, - verifyExactlyOneValid( + verifyExactlyOneVerifierValid( verifyResponseCause(abortCause), verifyResponseCause(AbortedStreamException.class) ) @@ -968,7 +968,7 @@ public void onComplete() {} TimeUnit.SECONDS.sleep(1L); // Sleep to check if there's a retry. assertThat(subscriberCancelServiceCallCounter.get()).isEqualTo(1); - awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + awaitValidClientRequestContext(ctx, verifyExactlyOneVerifierValid( verifyAllValid( verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), verifyResponseCause(AbortedStreamException.class) @@ -1065,7 +1065,7 @@ void exceptionInRule() { .hasCauseReference(exception); ctx = captor.get(); } - awaitValidClientRequestContext(ctx, verifyExactlyOneValid( + awaitValidClientRequestContext(ctx, verifyExactlyOneVerifierValid( verifyResponseCause(AbortedStreamException.class), verifyResponseCause(exception) )); diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index a63b79cfa72..ff2b942f905 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -54,7 +54,7 @@ public static RequestLogVerifier verifyAllValid(RequestLogVerifier... childLogVe }; } - public static RequestLogVerifier verifyExactlyOneValid(RequestLogVerifier... childLogVerifiers) { + public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifier... childLogVerifiers) { return childLog -> { final Throwable[] verifierCauses = new Throwable[childLogVerifiers.length]; From fe471751babda2c17023f29990c8b86597ea0115 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 12:44:29 +0200 Subject: [PATCH 08/12] refactor: remove skipping of verifiers when there are no child logs --- .../armeria/internal/testing/RequestContextUtils.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index ff2b942f905..5b77ef5f1a0 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -185,10 +185,6 @@ private static void assertValidRequestContextWithVerifier( assertThat(ctx.log().isComplete()).isTrue(); assertThat(ctx.log().children()).hasSize(expectedNumRequests); - if (expectedNumRequests == 0) { - return; - } - for (int childLogIndex = 0; childLogIndex < expectedNumRequests; childLogIndex++) { final RequestLog childLog = ctx.log().children().get(childLogIndex).whenComplete().join(); assertThat(childLog).isNotNull(); From 0841164fb6b2c49dc79bfc589184244319b4850c Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 12:44:50 +0200 Subject: [PATCH 09/12] style: remove unnecessary line break --- .../com/linecorp/armeria/client/retry/RetryingClientTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index c8b7de906e7..8697a4300fc 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -1078,8 +1078,7 @@ void exceptionInRuleWithContent() { throw exception; }; - final WebClient client = client(rule, - 10000, 0, 100); + final WebClient client = client(rule, 10000, 0, 100); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { assertThatThrownBy(client.get("/").aggregate()::join) .isInstanceOf(CompletionException.class) From 98b8aa6ee47f48552872f5beb0d1f124bb807370 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 12:50:41 +0200 Subject: [PATCH 10/12] refactor: rename `verifyAllValid` to `verifyAllVerifierValid` --- .../client/retry/RetryingClientTest.java | 26 +++++++++---------- .../internal/testing/RequestContextUtils.java | 6 ++--- .../client/retry/RetryingRpcClientTest.java | 14 +++++----- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index 8697a4300fc..c66e904b0e1 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -20,7 +20,7 @@ import static com.linecorp.armeria.common.util.Exceptions.peel; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllVerifierValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneVerifierValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseHeader; @@ -404,11 +404,11 @@ void retryWhenTrailerMatched() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); awaitValidClientRequestContext(ctx, - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.OK), verifyResponseTrailer("grpc-status", "3") ), - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.OK), verifyResponseTrailer("grpc-status", "0") ) @@ -469,7 +469,7 @@ void respectRetryAfter() { assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo( (long) (TimeUnit.SECONDS.toMillis(1) * 0.9)); awaitValidClientRequestContext(ctx, - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), verifyResponseHeader( HttpHeaderNames.RETRY_AFTER.toString(), @@ -499,9 +499,9 @@ void respectRetryAfterWithHttpDate() throws InterruptedException { // Since ZonedDateTime doesn't express exact time, // just check out whether it is retried after delayed some time. assertThat(sw.elapsed(TimeUnit.MILLISECONDS)).isGreaterThanOrEqualTo(1000); - awaitValidClientRequestContext(ctx, verifyAllValid(verifyStatusCode( + awaitValidClientRequestContext(ctx, verifyAllVerifierValid(verifyStatusCode( HttpStatus.SERVICE_UNAVAILABLE), - verifyResponseHeader( + verifyResponseHeader( HttpHeaderNames.RETRY_AFTER.toString(), expectedRetryAfterHeader)), verifyStatusCode(HttpStatus.OK)); @@ -560,9 +560,9 @@ void retryAfterOneYear() throws InterruptedException { .get(HttpHeaderNames.RETRY_AFTER.toString()); assertThat(expectedRetryAfterHeader).isNotNull(); - awaitValidClientRequestContext(ctx, verifyAllValid(verifyStatusCode( + awaitValidClientRequestContext(ctx, verifyAllVerifierValid(verifyStatusCode( HttpStatus.SERVICE_UNAVAILABLE), - verifyResponseHeader( + verifyResponseHeader( HttpHeaderNames.RETRY_AFTER.toString(), expectedRetryAfterHeader))); } @@ -587,7 +587,7 @@ void retryOnResponseTimeout() { } assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); - awaitValidClientRequestContext(ctx, verifyAllValid( + awaitValidClientRequestContext(ctx, verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(ResponseTimeoutException.class) ), verifyStatusCode(HttpStatus.OK)); @@ -619,7 +619,7 @@ void retryWithContentOnResponseTimeout() { assertThat(res.contentUtf8()).isEqualTo("Succeeded after retry"); // Make sure that all customized RetryRuleWithContents are called. assertThat(queue).containsExactly(1, 2, 3); - awaitValidClientRequestContext(ctx, verifyAllValid( + awaitValidClientRequestContext(ctx, verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(ResponseTimeoutException.class) ), @@ -877,7 +877,7 @@ void shouldGetExceptionWhenFactoryIsClosed() { } else { awaitValidClientRequestContext(ctx, verifyExactlyOneVerifierValid( verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(IllegalStateException.class) ) @@ -969,11 +969,11 @@ public void onComplete() {} TimeUnit.SECONDS.sleep(1L); // Sleep to check if there's a retry. assertThat(subscriberCancelServiceCallCounter.get()).isEqualTo(1); awaitValidClientRequestContext(ctx, verifyExactlyOneVerifierValid( - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.SERVICE_UNAVAILABLE), verifyResponseCause(AbortedStreamException.class) ), - verifyAllValid( + verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(CancelledSubscriptionException.class) ) diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index 5b77ef5f1a0..bdfea758312 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -46,7 +46,7 @@ public interface RequestLogVerifier { // No verification is performed. }; - public static RequestLogVerifier verifyAllValid(RequestLogVerifier... childLogVerifiers) { + public static RequestLogVerifier verifyAllVerifierValid(RequestLogVerifier... childLogVerifiers) { return childLog -> { for (RequestLogVerifier childLogVerifier : childLogVerifiers) { childLogVerifier.verifyLog(childLog); @@ -84,7 +84,7 @@ public static RequestLogVerifier verifyStatusCode(HttpStatus expectedStatus) { } public static RequestLogVerifier verifyUnprocessedRequestException() { - return verifyAllValid( + return verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(UnprocessedRequestException.class) ); @@ -154,7 +154,7 @@ private static void assertValidRequestContextWithVerifier( ctx, childLogVerifiers.length == 0 ? VERIFY_NOTHING - : verifyAllValid( + : verifyAllVerifierValid( childLog -> { // Default parent log verifier. final HttpRequest req = ctx.request(); diff --git a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java index 1af1b80793b..42c85123a98 100644 --- a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java +++ b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java @@ -18,7 +18,7 @@ import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllVerifierValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyRequestCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyStatusCode; @@ -277,7 +277,7 @@ void exceptionInStrategy() { } awaitValidRequestContextWithParentLogVerifier(ctx, - verifyAllValid( + verifyAllVerifierValid( // Not a response exception from the server so // we are not using verifyResponseCause verifyStatusCode(HttpStatus.UNKNOWN), @@ -372,7 +372,7 @@ void shouldGetExceptionWhenFactoryIsClosed() throws Exception { final Throwable cause = t.getCause(); assertThat(cause).isInstanceOf(IllegalStateException.class); t = cause; - awaitValidClientRequestContext(ctx, verifyAllValid( + awaitValidClientRequestContext(ctx, verifyAllVerifierValid( // We cannot be sure that we set // the request cause so we are not checking // with verifyRequestException/ @@ -382,7 +382,7 @@ void shouldGetExceptionWhenFactoryIsClosed() throws Exception { )); } else { awaitValidRequestContextWithParentLogVerifier(ctx, - verifyAllValid( + verifyAllVerifierValid( // Same as above. verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(t) @@ -443,7 +443,7 @@ private static void awaitValidRequestContextWithParentLogVerifier(ClientRequestC } private static RequestLogVerifier verifyRequestException(Class causeClass) { - return verifyAllValid( + return verifyAllVerifierValid( verifyStatusCode(HttpStatus.UNKNOWN), verifyRequestCause(causeClass), verifyResponseCause(causeClass) @@ -455,7 +455,7 @@ private static RequestLogVerifier verifyResponseException() { } private static RequestLogVerifier verifyResponseException(int type) { - return verifyAllValid( + return verifyAllVerifierValid( verifyStatusCode(HttpStatus.OK), verifyResponseCause(TApplicationException.class), childLog -> { @@ -468,7 +468,7 @@ private static RequestLogVerifier verifyResponseException(int type) { } private static RequestLogVerifier verifyResponse(@Nullable String expectedResponse) { - return verifyAllValid( + return verifyAllVerifierValid( verifyStatusCode(HttpStatus.OK), childLog -> { assertThat(childLog.responseContent()).isExactlyInstanceOf(CompletableRpcResponse.class); From 57b11abf85c7a63324eb6ab41b216a4466325ec5 Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 13:22:56 +0200 Subject: [PATCH 11/12] refactor: enforce that users provide a parent log verifier --- .../client/retry/RetryingClientTest.java | 12 ++- .../internal/testing/RequestContextUtils.java | 75 +++++++++---------- .../client/retry/RetryingRpcClientTest.java | 18 +++-- 3 files changed, 53 insertions(+), 52 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java index c66e904b0e1..2630d102125 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientTest.java @@ -19,9 +19,9 @@ import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; import static com.linecorp.armeria.common.util.Exceptions.peel; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllVerifierValid; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyExactlyOneVerifierValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyRequestExistsAndCompleted; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseHeader; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseTrailer; @@ -1174,15 +1174,19 @@ private WebClient client(RetryRuleWithContent retryRuleWithContent private static void awaitValidClientRequestContext(ClientRequestContext ctx, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidRequestContext(ctx, childLogVerifiers)); + await().untilAsserted(() -> + assertValidRequestContext( + ctx, verifyRequestExistsAndCompleted(), childLogVerifiers + ) + ); } public static void awaitValidClientRequestContextWithParentLogVerifier( ClientRequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidRequestContextWithParentLogVerifier(ctx, parentLogVerifier, - childLogVerifiers)); + await().untilAsserted(() -> assertValidRequestContext(ctx, parentLogVerifier, + childLogVerifiers)); } private static class RetryIfContentMatch implements RetryRuleWithContent { diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index bdfea758312..91ae54ce4e3 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -25,6 +25,7 @@ import java.util.Objects; import java.util.stream.Collectors; +import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.UnprocessedRequestException; import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpStatus; @@ -96,6 +97,38 @@ public static RequestLogVerifier verifyRequestCause(Class expectedCauseClass) }; } + public static RequestLogVerifier verifyRequestExistsAndCompleted() { + return requestLog -> { + final RequestContext requestContext = requestLog.context(); + assert requestContext instanceof ClientRequestContext; + final ClientRequestContext ctx = (ClientRequestContext) requestContext; + + final HttpRequest req = ctx.request(); + assertThat(req).isNotNull(); + assert req != null; + assertThat(req.isComplete()).isTrue(); + }; + } + + public static RequestLogVerifier verifyLastChildHasSameHttpRequest() { + return requestLog -> { + final RequestContext requestContext = requestLog.context(); + assert requestContext instanceof ClientRequestContext; + final ClientRequestContext ctx = (ClientRequestContext) requestContext; + + // We expect at least one child log. + assertThat(ctx.log().children().size()).isGreaterThanOrEqualTo(1); + + final HttpRequest lastHttpReq = + ctx.log().children() + .get(ctx.log().children().size() - 1).context().request(); + + if (lastHttpReq != null) { + assertThat(lastHttpReq).isSameAs(ctx.log().context().request()); + } + }; + } + public static RequestLogVerifier verifyResponseCause(Class expectedCauseClass) { return childLog -> { assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedCauseClass); @@ -130,53 +163,13 @@ public static RequestLogVerifier verifyResponseContent(String expectedResponseCo }; } - public static void assertValidRequestContext(RequestContext ctx, - RequestLogVerifier... childLogVerifiers) { - assertValidRequestContextWithVerifier(ctx, childLogVerifiers); - } - - public static void assertValidRequestContextWithParentLogVerifier( + public static void assertValidRequestContext( RequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { assertValidRequestContextWithVerifier(ctx, parentLogVerifier, childLogVerifiers); } - private static void assertValidRequestContextWithVerifier( - RequestContext ctx, - RequestLogVerifier[] childLogVerifiers) { - if (childLogVerifiers.length == 0) { - childLogVerifiers = new RequestLogVerifier[ctx.log().children().size()]; - Arrays.fill(childLogVerifiers, VERIFY_NOTHING); - } - - assertValidRequestContextWithVerifier( - ctx, - childLogVerifiers.length == 0 ? - VERIFY_NOTHING - : verifyAllVerifierValid( - childLog -> { - // Default parent log verifier. - final HttpRequest req = ctx.request(); - assertThat(req).isNotNull(); - assert req != null; - assertThat(req.isComplete()).isTrue(); - - if (ctx.rpcRequest() != null) { - final HttpRequest lastHttpReq = - ctx.log().children() - .get(ctx.log().children().size() - 1).context().request(); - - if (lastHttpReq != null) { - assertThat(lastHttpReq).isSameAs(ctx.log().context().request()); - } - } - }, - childLogVerifiers[childLogVerifiers.length - 1] - ), childLogVerifiers - ); - } - private static void assertValidRequestContextWithVerifier( RequestContext ctx, RequestLogVerifier parentLogVerifier, diff --git a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java index 42c85123a98..6d1c51c89cf 100644 --- a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java +++ b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/client/retry/RetryingRpcClientTest.java @@ -17,9 +17,10 @@ import static com.linecorp.armeria.client.retry.AbstractRetryingClient.ARMERIA_RETRY_COUNT; import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContext; -import static com.linecorp.armeria.internal.testing.RequestContextUtils.assertValidRequestContextWithParentLogVerifier; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyAllVerifierValid; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyLastChildHasSameHttpRequest; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyRequestCause; +import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyRequestExistsAndCompleted; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyResponseCause; import static com.linecorp.armeria.internal.testing.RequestContextUtils.verifyStatusCode; import static org.assertj.core.api.Assertions.assertThat; @@ -278,8 +279,6 @@ void exceptionInStrategy() { awaitValidRequestContextWithParentLogVerifier(ctx, verifyAllVerifierValid( - // Not a response exception from the server so - // we are not using verifyResponseCause verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(exception) ), @@ -383,7 +382,6 @@ void shouldGetExceptionWhenFactoryIsClosed() throws Exception { } else { awaitValidRequestContextWithParentLogVerifier(ctx, verifyAllVerifierValid( - // Same as above. verifyStatusCode(HttpStatus.UNKNOWN), verifyResponseCause(t) ), @@ -423,7 +421,7 @@ void doNotRetryWhenResponseIsCancelled() throws Exception { // Sleep 1 second more to check if there was another retry. TimeUnit.SECONDS.sleep(1); verify(serviceHandler, only()).hello("hello"); - assertValidRequestContextWithParentLogVerifier( + assertValidRequestContext( ctx, // ClientUtil.completeLogIfIncomplete() records exceptions caused by response cancellations. verifyRequestException(CancellationException.class), @@ -432,13 +430,19 @@ void doNotRetryWhenResponseIsCancelled() throws Exception { private static void awaitValidClientRequestContext(ClientRequestContext ctx, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidRequestContext(ctx, childLogVerifiers)); + await().untilAsserted(() -> assertValidRequestContext(ctx, + verifyAllVerifierValid( + verifyRequestExistsAndCompleted(), + verifyLastChildHasSameHttpRequest() + ), + childLogVerifiers) + ); } private static void awaitValidRequestContextWithParentLogVerifier(ClientRequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { - await().untilAsserted(() -> assertValidRequestContextWithParentLogVerifier( + await().untilAsserted(() -> assertValidRequestContext( ctx, parentLogVerifier, childLogVerifiers)); } From 590488675c703bc4e1cdbdc78f5d9d523fafdb0e Mon Sep 17 00:00:00 2001 From: "szymon.habrainski" Date: Wed, 2 Jul 2025 13:27:41 +0200 Subject: [PATCH 12/12] refactor: improve internal naming --- .../internal/testing/RequestContextUtils.java | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java index 91ae54ce4e3..e04277f7086 100644 --- a/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java +++ b/testing-internal/src/main/java/com/linecorp/armeria/internal/testing/RequestContextUtils.java @@ -43,32 +43,32 @@ public interface RequestLogVerifier { void verifyLog(RequestLog requestLog) throws Exception; } - public static final RequestLogVerifier VERIFY_NOTHING = childLog -> { + public static final RequestLogVerifier VERIFY_NOTHING = requestLog -> { // No verification is performed. }; - public static RequestLogVerifier verifyAllVerifierValid(RequestLogVerifier... childLogVerifiers) { - return childLog -> { - for (RequestLogVerifier childLogVerifier : childLogVerifiers) { - childLogVerifier.verifyLog(childLog); + public static RequestLogVerifier verifyAllVerifierValid(RequestLogVerifier... requestLogVerifiers) { + return requestLog -> { + for (RequestLogVerifier requestLogVerifier : requestLogVerifiers) { + requestLogVerifier.verifyLog(requestLog); } }; } - public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifier... childLogVerifiers) { - return childLog -> { - final Throwable[] verifierCauses = new Throwable[childLogVerifiers.length]; + public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifier... requestLogVerifiers) { + return requestLog -> { + final Throwable[] verifierCauses = new Throwable[requestLogVerifiers.length]; - for (int i = 0; i < childLogVerifiers.length; i++) { + for (int i = 0; i < requestLogVerifiers.length; i++) { final int index = i; - verifierCauses[i] = catchThrowable(() -> childLogVerifiers[index].verifyLog(childLog)); + verifierCauses[i] = catchThrowable(() -> requestLogVerifiers[index].verifyLog(requestLog)); } final List nonNullVerifierCauses = Arrays.stream(verifierCauses) .filter(Objects::nonNull) .collect(Collectors.toList()); - if (nonNullVerifierCauses.size() != childLogVerifiers.length - 1) { + if (nonNullVerifierCauses.size() != requestLogVerifiers.length - 1) { final Throwable allCauses = nonNullVerifierCauses.get(0); for (int i = 1; i < nonNullVerifierCauses.size(); i++) { @@ -81,7 +81,7 @@ public static RequestLogVerifier verifyExactlyOneVerifierValid(RequestLogVerifie } public static RequestLogVerifier verifyStatusCode(HttpStatus expectedStatus) { - return childLog -> assertThat(childLog.responseHeaders().status()).isEqualTo(expectedStatus); + return requestLog -> assertThat(requestLog.responseHeaders().status()).isEqualTo(expectedStatus); } public static RequestLogVerifier verifyUnprocessedRequestException() { @@ -92,8 +92,8 @@ public static RequestLogVerifier verifyUnprocessedRequestException() { } public static RequestLogVerifier verifyRequestCause(Class expectedCauseClass) { - return childLog -> { - assertThat(childLog.requestCause()).isExactlyInstanceOf(expectedCauseClass); + return requestLog -> { + assertThat(requestLog.requestCause()).isExactlyInstanceOf(expectedCauseClass); }; } @@ -130,36 +130,36 @@ public static RequestLogVerifier verifyLastChildHasSameHttpRequest() { } public static RequestLogVerifier verifyResponseCause(Class expectedCauseClass) { - return childLog -> { - assertThat(childLog.responseCause()).isExactlyInstanceOf(expectedCauseClass); + return requestLog -> { + assertThat(requestLog.responseCause()).isExactlyInstanceOf(expectedCauseClass); }; } public static RequestLogVerifier verifyResponseCause(Throwable expectedCause) { - return childLog -> { - assertThat(childLog.responseCause()).isSameAs(expectedCause); + return requestLog -> { + assertThat(requestLog.responseCause()).isSameAs(expectedCause); }; } public static RequestLogVerifier verifyResponseHeader(String headerName, String expectedHeaderValue) { - return childLog -> { - final ResponseHeaders headers = childLog.responseHeaders(); + return requestLog -> { + final ResponseHeaders headers = requestLog.responseHeaders(); assertThat(headers.get(headerName)).isEqualTo(expectedHeaderValue); }; } public static RequestLogVerifier verifyResponseTrailer(String headerName, String expectedHeaderValue) { - return childLog -> { - assertThat(childLog.responseTrailers().get(headerName)).isEqualTo(expectedHeaderValue); + return requestLog -> { + assertThat(requestLog.responseTrailers().get(headerName)).isEqualTo(expectedHeaderValue); }; } public static RequestLogVerifier verifyResponseContent(String expectedResponseContent) { - return childLog -> { - assertThat(childLog.responseContent()).isExactlyInstanceOf(String.class); - assertThat(childLog.responseContent()).isEqualTo(expectedResponseContent); + return requestLog -> { + assertThat(requestLog.responseContent()).isExactlyInstanceOf(String.class); + assertThat(requestLog.responseContent()).isEqualTo(expectedResponseContent); }; } @@ -167,10 +167,10 @@ public static void assertValidRequestContext( RequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier... childLogVerifiers) { - assertValidRequestContextWithVerifier(ctx, parentLogVerifier, childLogVerifiers); + assertValidRequestContext0(ctx, parentLogVerifier, childLogVerifiers); } - private static void assertValidRequestContextWithVerifier( + private static void assertValidRequestContext0( RequestContext ctx, RequestLogVerifier parentLogVerifier, RequestLogVerifier[] childLogVerifiers) {