From 51280666044288c6c248012b7decc347e44bd32e Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 1 Jul 2026 19:09:47 +0000 Subject: [PATCH 01/19] feat(client): add REQUEST_BODY_FACTORY attribute key for reproducible request bodies Co-authored-by: Isaac --- .../client/ClientRequestBodyFactory.java | 64 +++++++++++++++++++ .../client/ClientRequestBodyFactoryTest.java | 42 ++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java create mode 100644 core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java new file mode 100644 index 00000000000..b6ee06e4887 --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java @@ -0,0 +1,64 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client; + +import java.util.function.Supplier; + +import com.linecorp.armeria.common.ExchangeType; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.annotation.UnstableApi; + +import io.netty.util.AttributeKey; + +/** + * Holds the well-known {@link AttributeKey} used to supply a reproducible request body to + * {@code RetryingClient} and {@code RedirectingClient}. + * + *

For streaming requests larger than about 2 GiB, buffering the body in memory for replay is + * neither possible (an {@code int} size limit is exceeded) nor desirable. Instead, set a + * {@link Supplier} that regenerates the request (and thus a fresh body stream) on demand: + * + *

{@code
+ * final Supplier factory =
+ *         () -> HttpRequest.of(headers, StreamMessage.of(path));
+ * final RequestOptions options =
+ *         RequestOptions.builder()
+ *                       .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory)
+ *                       .build();
+ * client.execute(factory.get(), options);
+ * }
+ * + *

The factory is invoked once per retry attempt or redirect hop beyond the first. The first + * attempt uses the request passed to {@code execute(...)}. Each request produced by the factory + * must be equivalent to the original (same method and {@code content-length}); only the body + * stream should be freshly opened. + * + *

This attribute is honored only for streaming requests + * ({@link ExchangeType#isRequestStreaming()}). It is silently ignored for aggregated requests. + */ +@UnstableApi +public final class ClientRequestBodyFactory { + + /** + * The {@link AttributeKey} of the {@link Supplier} that regenerates the request body for each + * retry attempt or redirect hop. See {@link ClientRequestBodyFactory} for usage. + */ + public static final AttributeKey> REQUEST_BODY_FACTORY = + AttributeKey.valueOf(ClientRequestBodyFactory.class, "REQUEST_BODY_FACTORY"); + + private ClientRequestBodyFactory() {} +} diff --git a/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java b/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java new file mode 100644 index 00000000000..60655233056 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java @@ -0,0 +1,42 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; + +import io.netty.util.AttributeKey; + +class ClientRequestBodyFactoryTest { + + @Test + void keyIsStableAndTyped() { + final AttributeKey> key = ClientRequestBodyFactory.REQUEST_BODY_FACTORY; + assertThat(key).isNotNull(); + // Same key instance is reused (AttributeKey.valueOf is interned by name). + assertThat(key).isSameAs(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); + + final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/"); + assertThat(factory.get().method()).isEqualTo(HttpMethod.POST); + } +} From c9d269d0d6dda44191c2cbe3bb2826c2cd726055 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 1 Jul 2026 20:15:58 +0000 Subject: [PATCH 02/19] feat(client): add non-buffering RequestFactoryHttpRequestDuplicator Co-authored-by: Isaac --- .../RequestFactoryHttpRequestDuplicator.java | 128 ++++++++++++++++++ ...questFactoryHttpRequestDuplicatorTest.java | 127 +++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java create mode 100644 core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java new file mode 100644 index 00000000000..bd5bd173cd2 --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java @@ -0,0 +1,128 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client; + +import static java.util.Objects.requireNonNull; + +import java.util.function.Supplier; + +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpRequestDuplicator; +import com.linecorp.armeria.common.HttpRequestWriter; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.annotation.Nullable; + +/** + * An {@link HttpRequestDuplicator} that reproduces the request body without buffering it. + * + *

The first {@link #duplicate()} returns the original request passed to the client; every + * subsequent call obtains a fresh request from the supplied factory. This avoids the ~2 GiB + * {@code int} size limit and the memory cost of {@code DefaultStreamMessageDuplicator}, which + * buffers the whole body for replay. + */ +public final class RequestFactoryHttpRequestDuplicator implements HttpRequestDuplicator { + + private final HttpRequest originalReq; + private final Supplier factory; + + // Confined to a single thread: RetryingClient and RedirectingClient drive the duplicator + // sequentially from the request's event loop, so this flag needs no synchronization + // (mirrors how DefaultStreamMessageDuplicator confines its own state to an EventExecutor). + private boolean firstDuplicateIssued; + + public RequestFactoryHttpRequestDuplicator(HttpRequest originalReq, + Supplier factory) { + this.originalReq = requireNonNull(originalReq, "originalReq"); + this.factory = requireNonNull(factory, "factory"); + } + + @Override + public RequestHeaders headers() { + return originalReq.headers(); + } + + @Override + public HttpRequest duplicate() { + return duplicate0(null); + } + + @Override + public HttpRequest duplicate(RequestHeaders newHeaders) { + requireNonNull(newHeaders, "newHeaders"); + return duplicate0(newHeaders); + } + + private HttpRequest duplicate0(@Nullable RequestHeaders newHeaders) { + if (!firstDuplicateIssued) { + firstDuplicateIssued = true; + return withHeaders(originalReq, newHeaders); + } + + final HttpRequest next; + try { + next = factory.get(); + } catch (Throwable t) { + return failed(newHeaders != null ? newHeaders : headers(), t); + } + if (next == null) { + return failed(newHeaders != null ? newHeaders : headers(), new NullPointerException( + "The request body factory returned null.")); + } + return withHeaders(next, newHeaders); + } + + private static HttpRequest withHeaders(HttpRequest req, @Nullable RequestHeaders newHeaders) { + return newHeaders != null ? req.withHeaders(newHeaders) : req; + } + + private static HttpRequest failed(RequestHeaders headers, Throwable cause) { + // Return a request that fails when subscribed. It is aborted eagerly so that + // whenComplete() is completed exceptionally even before any subscription, which + // lets callers observe the failure without subscribing to the body. + final HttpRequestWriter failed = HttpRequest.streaming(headers); + failed.abort(cause); + return failed; + } + + @Override + public void close() { + abortOriginalIfUnused(null); + } + + @Override + public void abort() { + abortOriginalIfUnused(null); + } + + @Override + public void abort(@Nullable Throwable cause) { + abortOriginalIfUnused(cause); + } + + private void abortOriginalIfUnused(@Nullable Throwable cause) { + // If the original request has never been handed to the wire, abort it to avoid a leak. + // Once the wire owns the subscription, abort() on an already-subscribed stream is a no-op. + if (!firstDuplicateIssued) { + firstDuplicateIssued = true; + if (cause != null) { + originalReq.abort(cause); + } else { + originalReq.abort(); + } + } + } +} diff --git a/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java new file mode 100644 index 00000000000..77b7a29fd85 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java @@ -0,0 +1,127 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.RequestHeaders; + +class RequestFactoryHttpRequestDuplicatorTest { + + @Test + void firstDuplicateReturnsOriginalThenFactory() { + final AtomicInteger factoryCalls = new AtomicInteger(); + final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); + final Supplier factory = () -> { + factoryCalls.incrementAndGet(); + return HttpRequest.of(HttpMethod.POST, "/fromFactory"); + }; + + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + // First duplicate: uses the original request, factory not invoked. + final HttpRequest first = dup.duplicate(); + assertThat(first.path()).isEqualTo("/orig"); + assertThat(factoryCalls).hasValue(0); + + // Second duplicate: uses the factory. + final HttpRequest second = dup.duplicate(); + assertThat(second.path()).isEqualTo("/fromFactory"); + assertThat(factoryCalls).hasValue(1); + } + + @Test + void headerOverrideAppliedOnBothPaths() { + final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); + final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + final RequestHeaders overridden = RequestHeaders.of(HttpMethod.POST, "/orig", "x-attempt", "1"); + final HttpRequest first = dup.duplicate(overridden); + assertThat(first.headers().get("x-attempt")).isEqualTo("1"); + + final RequestHeaders overridden2 = RequestHeaders.of(HttpMethod.POST, "/orig", "x-attempt", "2"); + final HttpRequest second = dup.duplicate(overridden2); + assertThat(second.headers().get("x-attempt")).isEqualTo("2"); + } + + @Test + void factoryThrowingYieldsFailedRequestNotThrow() { + final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); + final RuntimeException boom = new RuntimeException("boom"); + final Supplier factory = () -> { + throw boom; + }; + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + dup.duplicate(); // first attempt consumes original + final HttpRequest failed = dup.duplicate(); // factory throws internally + // Does not throw; the returned request fails when subscribed. + assertThat(failed.whenComplete()).isCompletedExceptionally(); + } + + @Test + void factoryReturningNullYieldsFailedRequest() { + final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); + final Supplier factory = () -> null; + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + dup.duplicate(); + final HttpRequest failed = dup.duplicate(); + assertThat(failed.whenComplete()).isCompletedExceptionally(); + } + + @Test + void abortBeforeDuplicateAbortsOriginal() { + final HttpRequest original = HttpRequest.streaming(RequestHeaders.of(HttpMethod.POST, "/orig")); + final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + dup.abort(new RuntimeException("cleanup")); + assertThat(original.whenComplete()).isCompletedExceptionally(); + } + + @Test + void bufferingDuplicatorThrowsAtIntCapWhileFactoryDoesNot() { + // The factory duplicator never accumulates signal length, so no ContentTooLargeException + // is possible regardless of body size. This asserts the factory path is unaffected by the + // int cap that limits DefaultStreamMessageDuplicator (verified separately in that class's + // own tests). Here we simply confirm many large-"reported"-length duplications succeed. + final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); + final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); + final RequestFactoryHttpRequestDuplicator dup = + new RequestFactoryHttpRequestDuplicator(original, factory); + + // First (original) + several factory-produced duplicates, none throw. + assertThat(dup.duplicate()).isNotNull(); + for (int i = 0; i < 5; i++) { + assertThat(dup.duplicate()).isNotNull(); + } + } +} From 2a846e7d867f246221aee40052d9b64e35881b05 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 1 Jul 2026 20:30:09 +0000 Subject: [PATCH 03/19] feat(retry): use request body factory instead of buffering when attribute present Co-authored-by: Isaac --- .../armeria/client/retry/RetryingClient.java | 8 +- .../RetryingClientReproducibleBodyTest.java | 101 ++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java diff --git a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java index a1e3e238e7c..b28e6301b78 100644 --- a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java @@ -25,10 +25,12 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.function.Function; +import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.linecorp.armeria.client.ClientRequestBodyFactory; import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.HttpClient; import com.linecorp.armeria.client.ResponseTimeoutException; @@ -53,6 +55,7 @@ import com.linecorp.armeria.internal.client.AggregatedHttpRequestDuplicator; import com.linecorp.armeria.internal.client.ClientPendingThrowableUtil; import com.linecorp.armeria.internal.client.ClientRequestContextExtension; +import com.linecorp.armeria.internal.client.RequestFactoryHttpRequestDuplicator; import com.linecorp.armeria.internal.client.TruncatingHttpResponse; import io.netty.handler.codec.DateFormatter; @@ -245,7 +248,10 @@ protected HttpResponse doExecute(ClientRequestContext ctx, HttpRequest req) thro final CompletableFuture responseFuture = new CompletableFuture<>(); final HttpResponse res = HttpResponse.of(responseFuture, ctx.eventLoop()); if (ctx.exchangeType().isRequestStreaming()) { - final HttpRequestDuplicator reqDuplicator = req.toDuplicator(ctx.eventLoop().withoutContext(), 0); + final Supplier bodyFactory = ctx.attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); + final HttpRequestDuplicator reqDuplicator = + bodyFactory != null ? new RequestFactoryHttpRequestDuplicator(req, bodyFactory) + : req.toDuplicator(ctx.eventLoop().withoutContext(), 0); doExecute0(ctx, reqDuplicator, req, res, responseFuture); } else { req.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop())) diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java new file mode 100644 index 00000000000..1a15110f168 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java @@ -0,0 +1,101 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client.retry; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.client.ClientRequestBodyFactory; +import com.linecorp.armeria.client.RequestOptions; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.ExchangeType; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.stream.StreamMessage; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class RetryingClientReproducibleBodyTest { + + private static final AtomicInteger serverHits = new AtomicInteger(); + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service("/upload", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> { + final int hit = serverHits.incrementAndGet(); + if (hit == 1) { + // Fail the first attempt to trigger a retry. + return AggregatedHttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR) + .toHttpResponse(); + } + return AggregatedHttpResponse.of(HttpStatus.OK, ctx.request().contentType(), + agg.contentUtf8()).toHttpResponse(); + }))); + } + }; + + @Test + void factoryRegeneratesBodyForRetry() { + serverHits.set(0); + final AtomicInteger factoryCalls = new AtomicInteger(); + + final Supplier factory = () -> { + factoryCalls.incrementAndGet(); + return HttpRequest.of( + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), + StreamMessage.of(HttpData.ofUtf8("hello-body"))); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) + .build(); + + final AggregatedHttpResponse res = + client.execute(factory.get(), options).aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("hello-body"); + // Server was hit twice (fail then succeed); factory produced attempt #1 and the retry body. + assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); + // factory.get() once for the initial request + once for the retry. + assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); + } +} From d0d1d0d4453c81239a8d3214ca31c5949ae1f80c Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 1 Jul 2026 20:37:48 +0000 Subject: [PATCH 04/19] feat(redirect): use request body factory instead of buffering when attribute present Co-authored-by: Isaac --- .../armeria/client/RedirectingClient.java | 7 +- ...RedirectingClientReproducibleBodyTest.java | 86 +++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java diff --git a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java index 1f83fa3610f..8f7099a9a2c 100644 --- a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java @@ -28,6 +28,7 @@ import java.util.concurrent.CompletableFuture; import java.util.function.BiPredicate; import java.util.function.Function; +import java.util.function.Supplier; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; @@ -60,6 +61,7 @@ import com.linecorp.armeria.internal.client.AggregatedHttpRequestDuplicator; import com.linecorp.armeria.internal.client.ClientBuilderParamsUtil; import com.linecorp.armeria.internal.client.ClientUtil; +import com.linecorp.armeria.internal.client.RequestFactoryHttpRequestDuplicator; import com.linecorp.armeria.internal.common.util.TemporaryThreadLocals; import io.netty.util.NetUtil; @@ -142,7 +144,10 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex final HttpResponse res = HttpResponse.of(responseFuture, ctx.eventLoop()); final RedirectContext redirectCtx = new RedirectContext(ctx, req, res, responseFuture); if (ctx.exchangeType().isRequestStreaming()) { - final HttpRequestDuplicator reqDuplicator = req.toDuplicator(ctx.eventLoop().withoutContext(), 0); + final Supplier bodyFactory = ctx.attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); + final HttpRequestDuplicator reqDuplicator = + bodyFactory != null ? new RequestFactoryHttpRequestDuplicator(req, bodyFactory) + : req.toDuplicator(ctx.eventLoop().withoutContext(), 0); execute0(ctx, redirectCtx, reqDuplicator, true); } else { req.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop())) diff --git a/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java new file mode 100644 index 00000000000..0cffc312dda --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java @@ -0,0 +1,86 @@ +/* + * Copyright 2026 LINE Corporation + * + * LINE 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.client; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.ExchangeType; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.ResponseHeaders; +import com.linecorp.armeria.common.stream.StreamMessage; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class RedirectingClientReproducibleBodyTest { + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service("/first", (ctx, req) -> HttpResponse.of( + ResponseHeaders.of(HttpStatus.TEMPORARY_REDIRECT, + HttpHeaderNames.LOCATION, "/second"))); + sb.service("/second", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> + AggregatedHttpResponse.of(HttpStatus.OK, ctx.request().contentType(), + agg.contentUtf8()).toHttpResponse()))); + } + }; + + @Test + void factoryRegeneratesBodyForRedirect() { + final AtomicInteger factoryCalls = new AtomicInteger(); + final Supplier factory = () -> { + factoryCalls.incrementAndGet(); + return HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/first", + HttpHeaderNames.CONTENT_TYPE, + "text/plain; charset=utf-8"), + StreamMessage.of(HttpData.ofUtf8("redir-body"))); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .build(); + + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) + .build(); + + final AggregatedHttpResponse res = + client.execute(factory.get(), options).aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("redir-body"); + // factory.get() once for the initial request + once for the redirected hop. + assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); + } +} From 4ca6f45a7e90025455ba3f36df09aea85dab4206 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 1 Jul 2026 21:04:42 +0000 Subject: [PATCH 05/19] test(client): harden reproducible-body coverage; make firstDuplicateIssued volatile Follow-ups from the whole-branch review: - Mark RequestFactoryHttpRequestDuplicator.firstDuplicateIssued volatile for safe publication across the caller-thread -> event-loop hand-off (accesses are ordered via the async response future, but not confined to one thread). - Rename the factory-duplicator size test to reflect what it actually asserts. - Add end-to-end tests: factory throwing on a retry fails the request; a non-streaming request ignores the factory; a 303 See Other redirect drops the body (method -> GET) without invoking the factory again. - Reset the server-hit counter via @BeforeEach. Co-authored-by: Isaac --- .../RequestFactoryHttpRequestDuplicator.java | 10 ++- ...RedirectingClientReproducibleBodyTest.java | 39 ++++++++++ .../RetryingClientReproducibleBodyTest.java | 77 ++++++++++++++++++- ...questFactoryHttpRequestDuplicatorTest.java | 2 +- 4 files changed, 122 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java index bd5bd173cd2..2f31a7c0eab 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java +++ b/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java @@ -39,10 +39,12 @@ public final class RequestFactoryHttpRequestDuplicator implements HttpRequestDup private final HttpRequest originalReq; private final Supplier factory; - // Confined to a single thread: RetryingClient and RedirectingClient drive the duplicator - // sequentially from the request's event loop, so this flag needs no synchronization - // (mirrors how DefaultStreamMessageDuplicator confines its own state to an EventExecutor). - private boolean firstDuplicateIssued; + // RetryingClient and RedirectingClient drive the duplicator sequentially (never concurrently): + // the first duplicate() may run on the caller thread while later attempts run on the request's + // event loop, but each access happens-after the previous one via the async response future. + // Marked volatile for safe publication across that thread hand-off, since accesses are ordered + // but not confined to a single thread. + private volatile boolean firstDuplicateIssued; public RequestFactoryHttpRequestDuplicator(HttpRequest originalReq, Supplier factory) { diff --git a/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java index 0cffc312dda..497d99d352f 100644 --- a/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java @@ -31,6 +31,7 @@ import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; import com.linecorp.armeria.common.RequestHeaders; import com.linecorp.armeria.common.ResponseHeaders; import com.linecorp.armeria.common.stream.StreamMessage; @@ -50,6 +51,13 @@ protected void configure(ServerBuilder sb) { req.aggregate().thenApply(agg -> AggregatedHttpResponse.of(HttpStatus.OK, ctx.request().contentType(), agg.contentUtf8()).toHttpResponse()))); + // 303 See Other: the method is rewritten to GET and the body is dropped. + sb.service("/see-other", (ctx, req) -> HttpResponse.of( + ResponseHeaders.of(HttpStatus.SEE_OTHER, HttpHeaderNames.LOCATION, "/target"))); + sb.service("/target", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( + HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + req.method() + ":" + agg.contentUtf8()).toHttpResponse()))); } }; @@ -83,4 +91,35 @@ void factoryRegeneratesBodyForRedirect() { // factory.get() once for the initial request + once for the redirected hop. assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); } + + @Test + void seeOtherRedirectDropsBodyAndDoesNotThrow() { + final AtomicInteger factoryCalls = new AtomicInteger(); + final Supplier factory = () -> { + factoryCalls.incrementAndGet(); + return HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/see-other", + HttpHeaderNames.CONTENT_TYPE, + "text/plain; charset=utf-8"), + StreamMessage.of(HttpData.ofUtf8("see-other-body"))); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .build(); + + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) + .build(); + + final AggregatedHttpResponse res = + client.execute(factory.get(), options).aggregate().join(); + + // On a 303 the method is rewritten to GET and the body is dropped; the redirected GET has + // an empty body. The factory duplicator is aborted and never invoked again for the hop. + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("GET:"); + } } diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java index 1a15110f168..58378f25296 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java @@ -17,10 +17,12 @@ package com.linecorp.armeria.client.retry; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -63,9 +65,13 @@ protected void configure(ServerBuilder sb) { } }; + @BeforeEach + void resetServerHits() { + serverHits.set(0); + } + @Test void factoryRegeneratesBodyForRetry() { - serverHits.set(0); final AtomicInteger factoryCalls = new AtomicInteger(); final Supplier factory = () -> { @@ -98,4 +104,73 @@ void factoryRegeneratesBodyForRetry() { // factory.get() once for the initial request + once for the retry. assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); } + + @Test + void factoryThrowingOnRetryFailsTheRequest() { + final AtomicInteger factoryCalls = new AtomicInteger(); + + // The initial request succeeds as a body; the factory throws when asked to regenerate it + // for the retry. The request must then fail rather than hang or buffer. + final Supplier factory = () -> { + if (factoryCalls.getAndIncrement() == 0) { + return HttpRequest.of( + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), + StreamMessage.of(HttpData.ofUtf8("hello-body"))); + } + throw new IllegalStateException("cannot regenerate body"); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) + .build(); + + // The first attempt returns 500 (server hit #1), the retry asks the factory which throws, + // so the overall request fails. + assertThatThrownBy(() -> client.execute(factory.get(), options).aggregate().join()) + .isInstanceOf(Exception.class); + assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); + } + + @Test + void nonStreamingRequestIgnoresFactory() { + final AtomicInteger factoryCalls = new AtomicInteger(); + final Supplier factory = () -> { + factoryCalls.incrementAndGet(); + return HttpRequest.of( + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), + StreamMessage.of(HttpData.ofUtf8("hello-body"))); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + // A non-streaming exchange type must ignore the factory and buffer/aggregate as before. + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.UNARY) + .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) + .build(); + + final AggregatedHttpResponse res = + client.execute(factory.get(), options).aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("hello-body"); + // The factory is only ever called by the caller to build the initial request (once); + // the retry path buffers the aggregated body and never invokes the factory again. + assertThat(factoryCalls).hasValue(1); + } } diff --git a/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java index 77b7a29fd85..df31a996091 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java @@ -108,7 +108,7 @@ void abortBeforeDuplicateAbortsOriginal() { } @Test - void bufferingDuplicatorThrowsAtIntCapWhileFactoryDoesNot() { + void factoryDuplicatorHasNoSizeCapAcrossManyDuplicates() { // The factory duplicator never accumulates signal length, so no ContentTooLargeException // is possible regardless of body size. This asserts the factory path is unaffected by the // int cap that limits DefaultStreamMessageDuplicator (verified separately in that class's From c3928a1d951340fa6d928929f6a91964214fba8e Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 8 Jul 2026 00:14:04 +0000 Subject: [PATCH 06/19] refactor(client): redesign reproducible request bodies as a first-class request type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the attribute-based `ClientRequestBodyFactory.REQUEST_BODY_FACTORY` (a `Supplier` read out of band by RetryingClient and RedirectingClient) with a first-class `HttpRequest.reproducible(RequestHeaders, Supplier>)`. The returned `ReproducibleHttpRequest` overrides `toDuplicator()` to return a non-buffering `ReproducibleHttpRequestDuplicator`, so both clients drop their special-case selection block and call `req.toDuplicator(...)` unconditionally. Every attempt — including the first — is regenerated from the factory, so the caller's request is never itself put on the wire. This fixes the prior design's retry-after-mid-body-failure regression: because the first attempt used the live request, a mid-body transport failure completed its `whenComplete()` exceptionally and RetryingClient short-circuited all retries. The body factory is invoked lazily (never eagerly at construction): once per attempt on the retry/redirect path, or once on direct subscription when no duplicating decorator is present. Fixed headers are reused for every attempt so the request method and content-length cannot drift. The duplicator tracks the single last-produced request (volatile, for the caller-thread to event-loop hand-off) so abort() can release a produced-but-unsubscribed body; close() lets an in-flight subscribed request finish per the StreamMessageDuplicator contract. duplicate() fails fast (propagates) when the factory throws or returns null, so an unreproducible body terminates the request instead of burning the retry budget. Reproducibility applies at the outermost duplicating decorator only; inner hops see ordinary requests. Copyright headers on new files use "LY Corporation". Co-authored-by: Isaac --- .../client/ClientRequestBodyFactory.java | 64 ----- .../armeria/client/RedirectingClient.java | 12 +- .../armeria/client/retry/RetryingClient.java | 25 +- .../linecorp/armeria/common/HttpRequest.java | 49 ++++ .../common/ReproducibleHttpRequest.java | 104 ++++++++ .../RequestFactoryHttpRequestDuplicator.java | 130 ---------- .../ReproducibleHttpRequestDuplicator.java | 130 ++++++++++ .../client/ClientRequestBodyFactoryTest.java | 42 --- ...RedirectingClientReproducibleBodyTest.java | 125 --------- .../ReproducibleHttpRequestClientTest.java | 243 ++++++++++++++++++ .../ReproducibleHttpRequestRetryTest.java | 103 ++++++++ .../RetryingClientReproducibleBodyTest.java | 176 ------------- ...questFactoryHttpRequestDuplicatorTest.java | 127 --------- ...ReproducibleHttpRequestDuplicatorTest.java | 118 +++++++++ 14 files changed, 762 insertions(+), 686 deletions(-) delete mode 100644 core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java create mode 100644 core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java delete mode 100644 core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java create mode 100644 core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java delete mode 100644 core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java delete mode 100644 core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java create mode 100644 core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java create mode 100644 core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java delete mode 100644 core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java delete mode 100644 core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java create mode 100644 core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java deleted file mode 100644 index b6ee06e4887..00000000000 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestBodyFactory.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client; - -import java.util.function.Supplier; - -import com.linecorp.armeria.common.ExchangeType; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.annotation.UnstableApi; - -import io.netty.util.AttributeKey; - -/** - * Holds the well-known {@link AttributeKey} used to supply a reproducible request body to - * {@code RetryingClient} and {@code RedirectingClient}. - * - *

For streaming requests larger than about 2 GiB, buffering the body in memory for replay is - * neither possible (an {@code int} size limit is exceeded) nor desirable. Instead, set a - * {@link Supplier} that regenerates the request (and thus a fresh body stream) on demand: - * - *

{@code
- * final Supplier factory =
- *         () -> HttpRequest.of(headers, StreamMessage.of(path));
- * final RequestOptions options =
- *         RequestOptions.builder()
- *                       .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory)
- *                       .build();
- * client.execute(factory.get(), options);
- * }
- * - *

The factory is invoked once per retry attempt or redirect hop beyond the first. The first - * attempt uses the request passed to {@code execute(...)}. Each request produced by the factory - * must be equivalent to the original (same method and {@code content-length}); only the body - * stream should be freshly opened. - * - *

This attribute is honored only for streaming requests - * ({@link ExchangeType#isRequestStreaming()}). It is silently ignored for aggregated requests. - */ -@UnstableApi -public final class ClientRequestBodyFactory { - - /** - * The {@link AttributeKey} of the {@link Supplier} that regenerates the request body for each - * retry attempt or redirect hop. See {@link ClientRequestBodyFactory} for usage. - */ - public static final AttributeKey> REQUEST_BODY_FACTORY = - AttributeKey.valueOf(ClientRequestBodyFactory.class, "REQUEST_BODY_FACTORY"); - - private ClientRequestBodyFactory() {} -} diff --git a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java index 8f7099a9a2c..b17ce63f82d 100644 --- a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java @@ -28,7 +28,6 @@ import java.util.concurrent.CompletableFuture; import java.util.function.BiPredicate; import java.util.function.Function; -import java.util.function.Supplier; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; @@ -61,7 +60,6 @@ import com.linecorp.armeria.internal.client.AggregatedHttpRequestDuplicator; import com.linecorp.armeria.internal.client.ClientBuilderParamsUtil; import com.linecorp.armeria.internal.client.ClientUtil; -import com.linecorp.armeria.internal.client.RequestFactoryHttpRequestDuplicator; import com.linecorp.armeria.internal.common.util.TemporaryThreadLocals; import io.netty.util.NetUtil; @@ -144,10 +142,7 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex final HttpResponse res = HttpResponse.of(responseFuture, ctx.eventLoop()); final RedirectContext redirectCtx = new RedirectContext(ctx, req, res, responseFuture); if (ctx.exchangeType().isRequestStreaming()) { - final Supplier bodyFactory = ctx.attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); - final HttpRequestDuplicator reqDuplicator = - bodyFactory != null ? new RequestFactoryHttpRequestDuplicator(req, bodyFactory) - : req.toDuplicator(ctx.eventLoop().withoutContext(), 0); + final HttpRequestDuplicator reqDuplicator = req.toDuplicator(ctx.eventLoop().withoutContext(), 0); execute0(ctx, redirectCtx, reqDuplicator, true); } else { req.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop())) @@ -190,9 +185,12 @@ private void execute0(ClientRequestContext ctx, RedirectContext redirectCtx, return; } - final HttpRequest duplicateReq = reqDuplicator.duplicate(); + final HttpRequest duplicateReq; final ClientRequestContext derivedCtx; try { + // duplicate() may throw if the request body cannot be reproduced (see + // HttpRequest.reproducible); fail the request instead of following the redirect. + duplicateReq = reqDuplicator.duplicate(); derivedCtx = ClientUtil.newDerivedContext(ctx, duplicateReq, ctx.rpcRequest(), initialAttempt); } catch (Throwable t) { handleException(ctx, reqDuplicator, responseFuture, t, initialAttempt); diff --git a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java index b28e6301b78..cbea9d305f0 100644 --- a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java @@ -25,12 +25,10 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.function.Function; -import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.linecorp.armeria.client.ClientRequestBodyFactory; import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.HttpClient; import com.linecorp.armeria.client.ResponseTimeoutException; @@ -55,7 +53,6 @@ import com.linecorp.armeria.internal.client.AggregatedHttpRequestDuplicator; import com.linecorp.armeria.internal.client.ClientPendingThrowableUtil; import com.linecorp.armeria.internal.client.ClientRequestContextExtension; -import com.linecorp.armeria.internal.client.RequestFactoryHttpRequestDuplicator; import com.linecorp.armeria.internal.client.TruncatingHttpResponse; import io.netty.handler.codec.DateFormatter; @@ -248,10 +245,7 @@ protected HttpResponse doExecute(ClientRequestContext ctx, HttpRequest req) thro final CompletableFuture responseFuture = new CompletableFuture<>(); final HttpResponse res = HttpResponse.of(responseFuture, ctx.eventLoop()); if (ctx.exchangeType().isRequestStreaming()) { - final Supplier bodyFactory = ctx.attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); - final HttpRequestDuplicator reqDuplicator = - bodyFactory != null ? new RequestFactoryHttpRequestDuplicator(req, bodyFactory) - : req.toDuplicator(ctx.eventLoop().withoutContext(), 0); + final HttpRequestDuplicator reqDuplicator = req.toDuplicator(ctx.eventLoop().withoutContext(), 0); doExecute0(ctx, reqDuplicator, req, res, responseFuture); } else { req.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop())) @@ -302,16 +296,17 @@ private void doExecute0(ClientRequestContext ctx, HttpRequestDuplicator rootReqD } final HttpRequest duplicateReq; - if (initialAttempt) { - duplicateReq = rootReqDuplicator.duplicate(); - } else { - final RequestHeadersBuilder newHeaders = originalReq.headers().toBuilder(); - newHeaders.setInt(ARMERIA_RETRY_COUNT, totalAttempts - 1); - duplicateReq = rootReqDuplicator.duplicate(newHeaders.build()); - } - final ClientRequestContext derivedCtx; try { + // duplicate() may throw if the request body cannot be reproduced (see + // HttpRequest.reproducible); fail the request instead of retrying an unreproducible body. + if (initialAttempt) { + duplicateReq = rootReqDuplicator.duplicate(); + } else { + final RequestHeadersBuilder newHeaders = originalReq.headers().toBuilder(); + newHeaders.setInt(ARMERIA_RETRY_COUNT, totalAttempts - 1); + duplicateReq = rootReqDuplicator.duplicate(newHeaders.build()); + } derivedCtx = newDerivedContext(ctx, duplicateReq, ctx.rpcRequest(), initialAttempt); } catch (Throwable t) { handleException(ctx, rootReqDuplicator, future, t, initialAttempt); diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index 01a739afca1..689032af822 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -30,6 +30,7 @@ import java.util.concurrent.CompletionStage; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.Supplier; import org.reactivestreams.Publisher; import org.reactivestreams.Subscriber; @@ -357,6 +358,54 @@ static HttpRequest of(RequestHeaders headers, return of(headers, StreamMessage.of(stage, subscriberExecutor)); } + /** + * Creates a new {@link HttpRequest} whose body can be reproduced on demand, so that + * {@code RetryingClient} and {@code RedirectingClient} can resend it without buffering the whole + * body in memory. + * + *

For streaming requests larger than about 2 GiB, buffering the body for replay is neither + * possible (an {@code int} size limit is exceeded) nor desirable. Instead, supply a factory that + * opens a fresh body {@link StreamMessage} on demand: + * + *

{@code
+     * final RequestHeaders headers =
+     *         RequestHeaders.of(HttpMethod.POST, "/upload",
+     *                           HttpHeaderNames.CONTENT_TYPE, "application/octet-stream");
+     * final HttpRequest req = HttpRequest.reproducible(headers, () -> StreamMessage.of(path));
+     * final RequestOptions options =
+     *         RequestOptions.builder()
+     *                       .exchangeType(ExchangeType.REQUEST_STREAMING)
+     *                       .build();
+     * client.execute(req, options);
+     * }
+ * + *

The {@code bodyFactory} is invoked once per attempt (initial request, retry attempt, or + * redirect hop). The fixed {@code headers} are reused for every attempt; the factory regenerates + * only the body {@link StreamMessage}, so the request method and headers cannot drift between + * attempts. The body each factory invocation produces must match the declared + * {@link HttpHeaderNames#CONTENT_LENGTH} (if any), exactly as for any streaming + * {@link HttpRequest}. + * + *

Reproducible replay applies only to the outermost duplicating decorator (typically + * {@code RetryingClient}). Each attempt handed downstream is an ordinary {@link HttpRequest}; + * an inner decorator (e.g. a redirect within a single retry attempt) treats it as a normal + * request and may buffer it. It is honored for streaming requests + * ({@link ExchangeType#isRequestStreaming()}); an aggregated exchange type buffers the body as + * usual. + * + * @param headers the fixed {@link RequestHeaders} reused for every attempt + * @param bodyFactory produces a fresh body {@link StreamMessage} for each attempt; it must not + * return {@code null} + */ + @UnstableApi + static HttpRequest reproducible( + RequestHeaders headers, + Supplier> bodyFactory) { + requireNonNull(headers, "headers"); + requireNonNull(bodyFactory, "bodyFactory"); + return new ReproducibleHttpRequest(headers, bodyFactory); + } + /** * Returns a new {@link HttpRequestBuilder}. */ diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java new file mode 100644 index 00000000000..4794fd12dc9 --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -0,0 +1,104 @@ +/* + * Copyright 2026 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.common; + +import java.util.concurrent.CompletableFuture; +import java.util.function.Supplier; + +import org.reactivestreams.Publisher; + +import com.linecorp.armeria.common.stream.StreamMessage; +import com.linecorp.armeria.internal.common.ReproducibleHttpRequestDuplicator; +import com.linecorp.armeria.internal.common.stream.NonOverridableStreamMessageWrapper; + +import io.netty.util.concurrent.EventExecutor; + +/** + * An {@link HttpRequest} whose body can be reproduced on demand, so {@code RetryingClient} and + * {@code RedirectingClient} can resend it without buffering the whole body in memory. + * + *

See {@link HttpRequest#reproducible(RequestHeaders, Supplier)} for details and usage. + * + *

The body factory is invoked lazily: when this request is duplicated (the retry/redirect path), + * {@link #toDuplicator(EventExecutor, long)} returns a {@link ReproducibleHttpRequestDuplicator} that + * calls the factory once per attempt and this request's own delegate is never subscribed. When this + * request is subscribed directly (no retry/redirect decorator), the factory is invoked exactly once, + * on subscription, to produce the single body. Either way the factory is never called eagerly. + */ +final class ReproducibleHttpRequest + extends NonOverridableStreamMessageWrapper implements HttpRequest { + + private final RequestHeaders headers; + private final Supplier> bodyFactory; + + ReproducibleHttpRequest(RequestHeaders headers, + Supplier> bodyFactory) { + super(lazyBody(bodyFactory)); + this.headers = headers; + this.bodyFactory = bodyFactory; + } + + /** + * Returns a {@link StreamMessage} that invokes {@code bodyFactory} on its first (and only) + * subscription, so the factory is not called until this request is actually consumed directly. + */ + private static StreamMessage lazyBody( + Supplier> bodyFactory) { + return StreamMessage.of((Publisher) subscriber -> { + final StreamMessage body; + try { + body = bodyFactory.get(); + } catch (Throwable t) { + StreamMessage.aborted(t).subscribe(subscriber); + return; + } + if (body == null) { + StreamMessage.aborted( + new NullPointerException("bodyFactory.get() returned null.")).subscribe(subscriber); + return; + } + @SuppressWarnings("unchecked") + final StreamMessage cast = (StreamMessage) body; + cast.subscribe(subscriber); + }); + } + + @Override + public RequestHeaders headers() { + return headers; + } + + @SuppressWarnings("unchecked") + @Override + public CompletableFuture aggregate(AggregationOptions options) { + return super.aggregate(options); + } + + @Override + public HttpRequestDuplicator toDuplicator(EventExecutor executor) { + // Ignore the executor: the reproducible duplicator never buffers, so it needs no subscriber + // executor. Every duplicate() obtains a fresh body from the factory. + return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); + } + + @Override + public HttpRequestDuplicator toDuplicator(EventExecutor executor, long maxRequestLength) { + // maxRequestLength does not apply: this duplicator accumulates nothing, so there is no + // buffered length to cap. Each attempt streams a fresh body straight from the factory. + return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); + } +} diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java deleted file mode 100644 index 2f31a7c0eab..00000000000 --- a/core/src/main/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicator.java +++ /dev/null @@ -1,130 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client; - -import static java.util.Objects.requireNonNull; - -import java.util.function.Supplier; - -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.HttpRequestDuplicator; -import com.linecorp.armeria.common.HttpRequestWriter; -import com.linecorp.armeria.common.RequestHeaders; -import com.linecorp.armeria.common.annotation.Nullable; - -/** - * An {@link HttpRequestDuplicator} that reproduces the request body without buffering it. - * - *

The first {@link #duplicate()} returns the original request passed to the client; every - * subsequent call obtains a fresh request from the supplied factory. This avoids the ~2 GiB - * {@code int} size limit and the memory cost of {@code DefaultStreamMessageDuplicator}, which - * buffers the whole body for replay. - */ -public final class RequestFactoryHttpRequestDuplicator implements HttpRequestDuplicator { - - private final HttpRequest originalReq; - private final Supplier factory; - - // RetryingClient and RedirectingClient drive the duplicator sequentially (never concurrently): - // the first duplicate() may run on the caller thread while later attempts run on the request's - // event loop, but each access happens-after the previous one via the async response future. - // Marked volatile for safe publication across that thread hand-off, since accesses are ordered - // but not confined to a single thread. - private volatile boolean firstDuplicateIssued; - - public RequestFactoryHttpRequestDuplicator(HttpRequest originalReq, - Supplier factory) { - this.originalReq = requireNonNull(originalReq, "originalReq"); - this.factory = requireNonNull(factory, "factory"); - } - - @Override - public RequestHeaders headers() { - return originalReq.headers(); - } - - @Override - public HttpRequest duplicate() { - return duplicate0(null); - } - - @Override - public HttpRequest duplicate(RequestHeaders newHeaders) { - requireNonNull(newHeaders, "newHeaders"); - return duplicate0(newHeaders); - } - - private HttpRequest duplicate0(@Nullable RequestHeaders newHeaders) { - if (!firstDuplicateIssued) { - firstDuplicateIssued = true; - return withHeaders(originalReq, newHeaders); - } - - final HttpRequest next; - try { - next = factory.get(); - } catch (Throwable t) { - return failed(newHeaders != null ? newHeaders : headers(), t); - } - if (next == null) { - return failed(newHeaders != null ? newHeaders : headers(), new NullPointerException( - "The request body factory returned null.")); - } - return withHeaders(next, newHeaders); - } - - private static HttpRequest withHeaders(HttpRequest req, @Nullable RequestHeaders newHeaders) { - return newHeaders != null ? req.withHeaders(newHeaders) : req; - } - - private static HttpRequest failed(RequestHeaders headers, Throwable cause) { - // Return a request that fails when subscribed. It is aborted eagerly so that - // whenComplete() is completed exceptionally even before any subscription, which - // lets callers observe the failure without subscribing to the body. - final HttpRequestWriter failed = HttpRequest.streaming(headers); - failed.abort(cause); - return failed; - } - - @Override - public void close() { - abortOriginalIfUnused(null); - } - - @Override - public void abort() { - abortOriginalIfUnused(null); - } - - @Override - public void abort(@Nullable Throwable cause) { - abortOriginalIfUnused(cause); - } - - private void abortOriginalIfUnused(@Nullable Throwable cause) { - // If the original request has never been handed to the wire, abort it to avoid a leak. - // Once the wire owns the subscription, abort() on an already-subscribed stream is a no-op. - if (!firstDuplicateIssued) { - firstDuplicateIssued = true; - if (cause != null) { - originalReq.abort(cause); - } else { - originalReq.abort(); - } - } - } -} diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java new file mode 100644 index 00000000000..875b3a0739c --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java @@ -0,0 +1,130 @@ +/* + * Copyright 2026 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.common; + +import static java.util.Objects.requireNonNull; + +import java.util.function.Supplier; + +import com.linecorp.armeria.common.HttpObject; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpRequestDuplicator; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.common.stream.StreamMessage; + +/** + * An {@link HttpRequestDuplicator} that reproduces the request body without buffering it. Every + * {@link #duplicate()} — including the first — obtains a fresh body from the supplied factory, so no + * attempt reuses another attempt's stream and the original request handed to the client is never put + * on the wire. This avoids the ~2 GiB {@code int} size limit and the memory cost of + * {@code DefaultStreamMessageDuplicator}, which buffers the whole body for replay. + * + *

{@code RetryingClient} and {@code RedirectingClient} drive this duplicator strictly + * sequentially: at most one produced request is outstanding at a time, and each access + * happens-after the previous one via the async response future. The duplicator tracks the single + * last-produced request so that {@link #abort(Throwable)} can tear it down if it was produced but + * never subscribed (e.g. endpoint selection threw before the wire took ownership). The fields are + * {@code volatile} for safe publication across the caller-thread → event-loop hand-off, matching how + * the clients drive it. + */ +public final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { + + private final RequestHeaders headers; + private final Supplier> bodyFactory; + + @Nullable + private volatile HttpRequest lastProduced; + private volatile boolean closed; + + public ReproducibleHttpRequestDuplicator( + RequestHeaders headers, + Supplier> bodyFactory) { + this.headers = requireNonNull(headers, "headers"); + this.bodyFactory = requireNonNull(bodyFactory, "bodyFactory"); + } + + @Override + public RequestHeaders headers() { + return headers; + } + + @Override + public HttpRequest duplicate() { + return duplicate(headers); + } + + @Override + public HttpRequest duplicate(RequestHeaders newHeaders) { + requireNonNull(newHeaders, "newHeaders"); + if (closed) { + throw new IllegalStateException("duplicator is closed or aborted."); + } + + // A produced-but-unsubscribed request from a previous attempt (e.g. endpoint selection threw + // before the wire subscribed) would otherwise be lost when we overwrite lastProduced; abort it + // so its body is released. Aborting an already-subscribed stream is a no-op. + final HttpRequest previous = lastProduced; + if (previous != null) { + previous.abort(); + } + + // Fail fast on a broken factory: propagate to the caller (RetryingClient / RedirectingClient), + // which completes the response exceptionally instead of re-judging an aborted request and + // wasting the retry budget. + final StreamMessage body = + requireNonNull(bodyFactory.get(), "bodyFactory.get() returned null."); + final HttpRequest produced = HttpRequest.of(newHeaders, body); + lastProduced = produced; + return produced; + } + + @Override + public void close() { + // Let the last-produced request finish: on the success path the wire owns it and is still + // streaming, and the StreamMessageDuplicator contract says close() must not abort issued + // duplicates. If it was never subscribed, it is completed/aborted by its own consumer or by a + // subsequent abort(); we intentionally do not abort it here. + closed = true; + } + + @Override + public void abort() { + abortLastProduced(null); + } + + @Override + public void abort(Throwable cause) { + abortLastProduced(requireNonNull(cause, "cause")); + } + + private void abortLastProduced(@Nullable Throwable cause) { + closed = true; + final HttpRequest lastProduced = this.lastProduced; + if (lastProduced == null) { + return; + } + this.lastProduced = null; + // If the wire already owns the subscription, abort() on an already-subscribed stream is a + // no-op. Otherwise this releases the produced-but-unsubscribed body (e.g. an open file). + if (cause != null) { + lastProduced.abort(cause); + } else { + lastProduced.abort(); + } + } +} diff --git a/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java b/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java deleted file mode 100644 index 60655233056..00000000000 --- a/core/src/test/java/com/linecorp/armeria/client/ClientRequestBodyFactoryTest.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.function.Supplier; - -import org.junit.jupiter.api.Test; - -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; - -import io.netty.util.AttributeKey; - -class ClientRequestBodyFactoryTest { - - @Test - void keyIsStableAndTyped() { - final AttributeKey> key = ClientRequestBodyFactory.REQUEST_BODY_FACTORY; - assertThat(key).isNotNull(); - // Same key instance is reused (AttributeKey.valueOf is interned by name). - assertThat(key).isSameAs(ClientRequestBodyFactory.REQUEST_BODY_FACTORY); - - final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/"); - assertThat(factory.get().method()).isEqualTo(HttpMethod.POST); - } -} diff --git a/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java deleted file mode 100644 index 497d99d352f..00000000000 --- a/core/src/test/java/com/linecorp/armeria/client/RedirectingClientReproducibleBodyTest.java +++ /dev/null @@ -1,125 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; - -import com.linecorp.armeria.common.AggregatedHttpResponse; -import com.linecorp.armeria.common.ExchangeType; -import com.linecorp.armeria.common.HttpData; -import com.linecorp.armeria.common.HttpHeaderNames; -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.HttpResponse; -import com.linecorp.armeria.common.HttpStatus; -import com.linecorp.armeria.common.MediaType; -import com.linecorp.armeria.common.RequestHeaders; -import com.linecorp.armeria.common.ResponseHeaders; -import com.linecorp.armeria.common.stream.StreamMessage; -import com.linecorp.armeria.server.ServerBuilder; -import com.linecorp.armeria.testing.junit5.server.ServerExtension; - -class RedirectingClientReproducibleBodyTest { - - @RegisterExtension - static final ServerExtension server = new ServerExtension() { - @Override - protected void configure(ServerBuilder sb) { - sb.service("/first", (ctx, req) -> HttpResponse.of( - ResponseHeaders.of(HttpStatus.TEMPORARY_REDIRECT, - HttpHeaderNames.LOCATION, "/second"))); - sb.service("/second", (ctx, req) -> HttpResponse.of( - req.aggregate().thenApply(agg -> - AggregatedHttpResponse.of(HttpStatus.OK, ctx.request().contentType(), - agg.contentUtf8()).toHttpResponse()))); - // 303 See Other: the method is rewritten to GET and the body is dropped. - sb.service("/see-other", (ctx, req) -> HttpResponse.of( - ResponseHeaders.of(HttpStatus.SEE_OTHER, HttpHeaderNames.LOCATION, "/target"))); - sb.service("/target", (ctx, req) -> HttpResponse.of( - req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( - HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, - req.method() + ":" + agg.contentUtf8()).toHttpResponse()))); - } - }; - - @Test - void factoryRegeneratesBodyForRedirect() { - final AtomicInteger factoryCalls = new AtomicInteger(); - final Supplier factory = () -> { - factoryCalls.incrementAndGet(); - return HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/first", - HttpHeaderNames.CONTENT_TYPE, - "text/plain; charset=utf-8"), - StreamMessage.of(HttpData.ofUtf8("redir-body"))); - }; - - final WebClient client = - WebClient.builder(server.httpUri()) - .followRedirects() - .build(); - - final RequestOptions options = - RequestOptions.builder() - .exchangeType(ExchangeType.REQUEST_STREAMING) - .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) - .build(); - - final AggregatedHttpResponse res = - client.execute(factory.get(), options).aggregate().join(); - - assertThat(res.status()).isEqualTo(HttpStatus.OK); - assertThat(res.contentUtf8()).isEqualTo("redir-body"); - // factory.get() once for the initial request + once for the redirected hop. - assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); - } - - @Test - void seeOtherRedirectDropsBodyAndDoesNotThrow() { - final AtomicInteger factoryCalls = new AtomicInteger(); - final Supplier factory = () -> { - factoryCalls.incrementAndGet(); - return HttpRequest.of(RequestHeaders.of(HttpMethod.POST, "/see-other", - HttpHeaderNames.CONTENT_TYPE, - "text/plain; charset=utf-8"), - StreamMessage.of(HttpData.ofUtf8("see-other-body"))); - }; - - final WebClient client = - WebClient.builder(server.httpUri()) - .followRedirects() - .build(); - - final RequestOptions options = - RequestOptions.builder() - .exchangeType(ExchangeType.REQUEST_STREAMING) - .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) - .build(); - - final AggregatedHttpResponse res = - client.execute(factory.get(), options).aggregate().join(); - - // On a 303 the method is rewritten to GET and the body is dropped; the redirected GET has - // an empty body. The factory duplicator is aborted and never invoked again for the hop. - assertThat(res.status()).isEqualTo(HttpStatus.OK); - assertThat(res.contentUtf8()).isEqualTo("GET:"); - } -} diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java new file mode 100644 index 00000000000..c31e0e51e2e --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -0,0 +1,243 @@ +/* + * Copyright 2026 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.client; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.client.retry.RetryRule; +import com.linecorp.armeria.client.retry.RetryingClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.ExchangeType; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpObject; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.ResponseHeaders; +import com.linecorp.armeria.common.stream.StreamMessage; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class ReproducibleHttpRequestClientTest { + + private static final AtomicInteger serverHits = new AtomicInteger(); + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service("/upload", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> { + final int hit = serverHits.incrementAndGet(); + if (hit == 1) { + // Fail the first attempt to trigger a retry. + return AggregatedHttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR) + .toHttpResponse(); + } + return AggregatedHttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8()).toHttpResponse(); + }))); + // Redirect chain: /first -> /second, echoing the body at /second. + sb.service("/first", (ctx, req) -> HttpResponse.of( + ResponseHeaders.of(HttpStatus.TEMPORARY_REDIRECT, + HttpHeaderNames.LOCATION, "/second"))); + sb.service("/second", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( + HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8()).toHttpResponse()))); + // 303 See Other: method rewritten to GET and body dropped. + sb.service("/see-other", (ctx, req) -> HttpResponse.of( + ResponseHeaders.of(HttpStatus.SEE_OTHER, HttpHeaderNames.LOCATION, "/target"))); + sb.service("/target", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( + HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + req.method() + ":" + agg.contentUtf8()).toHttpResponse()))); + } + }; + + @BeforeEach + void resetServerHits() { + serverHits.set(0); + } + + private static RequestOptions streamingOptions() { + return RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .build(); + } + + @Test + void factoryNotInvokedEagerly() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/upload"); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + }; + + // Creating the request must not call the factory; it is invoked lazily per attempt only. + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + assertThat(bodyCalls).hasValue(0); + req.abort(); + } + + @Test + void retryRegeneratesBody() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("hello-body"); + assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); + // Body regenerated for the initial attempt and the retry. + assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + } + + @Test + void factoryThrowingOnRetryFailsFast() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + // Initial body succeeds; the factory throws when asked to regenerate it for the retry. + final Supplier> bodyFactory = () -> { + if (bodyCalls.getAndIncrement() == 0) { + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + } + throw new IllegalStateException("cannot regenerate body"); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + streamingOptions()) + .aggregate().join()) + .isInstanceOf(Exception.class); + // The factory is consulted twice: once for the initial body (via reproducible()), once for the + // failing retry. It fails fast rather than looping through the whole retry budget. + assertThat(bodyCalls).hasValue(2); + } + + @Test + void followsRedirectRegeneratingBody() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/first", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("redir-body")); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("redir-body"); + // Body regenerated for the initial request and the redirected hop. + assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + } + + @Test + void seeOtherRedirectDropsBody() { + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/see-other", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = + () -> StreamMessage.of(HttpData.ofUtf8("see-other-body")); + + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + // On a 303 the method is rewritten to GET and the body dropped; the duplicator is aborted + // and must not throw. + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("GET:"); + } + + @Test + void stackedRetryAndRedirect() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/first", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("redir-body")); + }; + + // Both RetryingClient and RedirectingClient present; the reproducible body must be resent + // correctly through the redirect without a destructive double-consume. + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("redir-body"); + } +} diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java new file mode 100644 index 00000000000..4bd2af1e86a --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java @@ -0,0 +1,103 @@ +/* + * Copyright 2026 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.client.retry; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.client.RequestOptions; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.ExchangeType; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpObject; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.stream.StreamMessage; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class ReproducibleHttpRequestRetryTest { + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service("/upload", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( + HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8()).toHttpResponse()))); + } + }; + + /** + * Regression test for the retry-after-mid-body-failure bug. The body supplier faults the FIRST + * attempt's request body (as if a mid-body transport failure occurred) and produces a good body + * on every subsequent attempt. A correct implementation regenerates the body for the retry and + * succeeds; the buggy implementation (where attempt #1 uses the caller's request as the live wire + * stream) faults the template request's {@code whenComplete()} and skips the retry entirely. + */ + @Test + void retriesAfterFirstAttemptBodyFailsMidStream() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodySupplier = () -> { + final int call = bodyCalls.incrementAndGet(); + if (call == 1) { + // Emit one chunk, then fault the request body mid-stream. + return StreamMessage.concat( + StreamMessage.of(HttpData.ofUtf8("partial")), + StreamMessage.aborted(new RuntimeException("mid-body failure"))); + } + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder() + .onException() + .onServerErrorStatus() + .thenBackoff())) + .build(); + + final RequestOptions options = + RequestOptions.builder() + .exchangeType(ExchangeType.REQUEST_STREAMING) + .build(); + + final HttpRequest req = HttpRequest.reproducible(headers, bodySupplier); + final AggregatedHttpResponse res = client.execute(req, options).aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("hello-body"); + // Body regenerated at least twice: the faulted first attempt and the successful retry. + assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + } +} diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java deleted file mode 100644 index 58378f25296..00000000000 --- a/core/src/test/java/com/linecorp/armeria/client/retry/RetryingClientReproducibleBodyTest.java +++ /dev/null @@ -1,176 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client.retry; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; - -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; - -import com.linecorp.armeria.client.ClientRequestBodyFactory; -import com.linecorp.armeria.client.RequestOptions; -import com.linecorp.armeria.client.WebClient; -import com.linecorp.armeria.common.AggregatedHttpResponse; -import com.linecorp.armeria.common.ExchangeType; -import com.linecorp.armeria.common.HttpData; -import com.linecorp.armeria.common.HttpHeaderNames; -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.HttpResponse; -import com.linecorp.armeria.common.HttpStatus; -import com.linecorp.armeria.common.MediaType; -import com.linecorp.armeria.common.RequestHeaders; -import com.linecorp.armeria.common.stream.StreamMessage; -import com.linecorp.armeria.server.ServerBuilder; -import com.linecorp.armeria.testing.junit5.server.ServerExtension; - -class RetryingClientReproducibleBodyTest { - - private static final AtomicInteger serverHits = new AtomicInteger(); - - @RegisterExtension - static final ServerExtension server = new ServerExtension() { - @Override - protected void configure(ServerBuilder sb) { - sb.service("/upload", (ctx, req) -> HttpResponse.of( - req.aggregate().thenApply(agg -> { - final int hit = serverHits.incrementAndGet(); - if (hit == 1) { - // Fail the first attempt to trigger a retry. - return AggregatedHttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR) - .toHttpResponse(); - } - return AggregatedHttpResponse.of(HttpStatus.OK, ctx.request().contentType(), - agg.contentUtf8()).toHttpResponse(); - }))); - } - }; - - @BeforeEach - void resetServerHits() { - serverHits.set(0); - } - - @Test - void factoryRegeneratesBodyForRetry() { - final AtomicInteger factoryCalls = new AtomicInteger(); - - final Supplier factory = () -> { - factoryCalls.incrementAndGet(); - return HttpRequest.of( - RequestHeaders.of(HttpMethod.POST, "/upload", - HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), - StreamMessage.of(HttpData.ofUtf8("hello-body"))); - }; - - final WebClient client = - WebClient.builder(server.httpUri()) - .decorator(RetryingClient.newDecorator( - RetryRule.builder().onServerErrorStatus().thenBackoff())) - .build(); - - final RequestOptions options = - RequestOptions.builder() - .exchangeType(ExchangeType.REQUEST_STREAMING) - .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) - .build(); - - final AggregatedHttpResponse res = - client.execute(factory.get(), options).aggregate().join(); - - assertThat(res.status()).isEqualTo(HttpStatus.OK); - assertThat(res.contentUtf8()).isEqualTo("hello-body"); - // Server was hit twice (fail then succeed); factory produced attempt #1 and the retry body. - assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); - // factory.get() once for the initial request + once for the retry. - assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); - } - - @Test - void factoryThrowingOnRetryFailsTheRequest() { - final AtomicInteger factoryCalls = new AtomicInteger(); - - // The initial request succeeds as a body; the factory throws when asked to regenerate it - // for the retry. The request must then fail rather than hang or buffer. - final Supplier factory = () -> { - if (factoryCalls.getAndIncrement() == 0) { - return HttpRequest.of( - RequestHeaders.of(HttpMethod.POST, "/upload", - HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), - StreamMessage.of(HttpData.ofUtf8("hello-body"))); - } - throw new IllegalStateException("cannot regenerate body"); - }; - - final WebClient client = - WebClient.builder(server.httpUri()) - .decorator(RetryingClient.newDecorator( - RetryRule.builder().onServerErrorStatus().thenBackoff())) - .build(); - - final RequestOptions options = - RequestOptions.builder() - .exchangeType(ExchangeType.REQUEST_STREAMING) - .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) - .build(); - - // The first attempt returns 500 (server hit #1), the retry asks the factory which throws, - // so the overall request fails. - assertThatThrownBy(() -> client.execute(factory.get(), options).aggregate().join()) - .isInstanceOf(Exception.class); - assertThat(factoryCalls).hasValueGreaterThanOrEqualTo(2); - } - - @Test - void nonStreamingRequestIgnoresFactory() { - final AtomicInteger factoryCalls = new AtomicInteger(); - final Supplier factory = () -> { - factoryCalls.incrementAndGet(); - return HttpRequest.of( - RequestHeaders.of(HttpMethod.POST, "/upload", - HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8), - StreamMessage.of(HttpData.ofUtf8("hello-body"))); - }; - - final WebClient client = - WebClient.builder(server.httpUri()) - .decorator(RetryingClient.newDecorator( - RetryRule.builder().onServerErrorStatus().thenBackoff())) - .build(); - - // A non-streaming exchange type must ignore the factory and buffer/aggregate as before. - final RequestOptions options = - RequestOptions.builder() - .exchangeType(ExchangeType.UNARY) - .attr(ClientRequestBodyFactory.REQUEST_BODY_FACTORY, factory) - .build(); - - final AggregatedHttpResponse res = - client.execute(factory.get(), options).aggregate().join(); - - assertThat(res.status()).isEqualTo(HttpStatus.OK); - assertThat(res.contentUtf8()).isEqualTo("hello-body"); - // The factory is only ever called by the caller to build the initial request (once); - // the retry path buffers the aggregated body and never invokes the factory again. - assertThat(factoryCalls).hasValue(1); - } -} diff --git a/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java deleted file mode 100644 index df31a996091..00000000000 --- a/core/src/test/java/com/linecorp/armeria/internal/client/RequestFactoryHttpRequestDuplicatorTest.java +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright 2026 LINE Corporation - * - * LINE 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.client; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; - -import org.junit.jupiter.api.Test; - -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.RequestHeaders; - -class RequestFactoryHttpRequestDuplicatorTest { - - @Test - void firstDuplicateReturnsOriginalThenFactory() { - final AtomicInteger factoryCalls = new AtomicInteger(); - final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); - final Supplier factory = () -> { - factoryCalls.incrementAndGet(); - return HttpRequest.of(HttpMethod.POST, "/fromFactory"); - }; - - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - // First duplicate: uses the original request, factory not invoked. - final HttpRequest first = dup.duplicate(); - assertThat(first.path()).isEqualTo("/orig"); - assertThat(factoryCalls).hasValue(0); - - // Second duplicate: uses the factory. - final HttpRequest second = dup.duplicate(); - assertThat(second.path()).isEqualTo("/fromFactory"); - assertThat(factoryCalls).hasValue(1); - } - - @Test - void headerOverrideAppliedOnBothPaths() { - final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); - final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - final RequestHeaders overridden = RequestHeaders.of(HttpMethod.POST, "/orig", "x-attempt", "1"); - final HttpRequest first = dup.duplicate(overridden); - assertThat(first.headers().get("x-attempt")).isEqualTo("1"); - - final RequestHeaders overridden2 = RequestHeaders.of(HttpMethod.POST, "/orig", "x-attempt", "2"); - final HttpRequest second = dup.duplicate(overridden2); - assertThat(second.headers().get("x-attempt")).isEqualTo("2"); - } - - @Test - void factoryThrowingYieldsFailedRequestNotThrow() { - final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); - final RuntimeException boom = new RuntimeException("boom"); - final Supplier factory = () -> { - throw boom; - }; - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - dup.duplicate(); // first attempt consumes original - final HttpRequest failed = dup.duplicate(); // factory throws internally - // Does not throw; the returned request fails when subscribed. - assertThat(failed.whenComplete()).isCompletedExceptionally(); - } - - @Test - void factoryReturningNullYieldsFailedRequest() { - final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); - final Supplier factory = () -> null; - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - dup.duplicate(); - final HttpRequest failed = dup.duplicate(); - assertThat(failed.whenComplete()).isCompletedExceptionally(); - } - - @Test - void abortBeforeDuplicateAbortsOriginal() { - final HttpRequest original = HttpRequest.streaming(RequestHeaders.of(HttpMethod.POST, "/orig")); - final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - dup.abort(new RuntimeException("cleanup")); - assertThat(original.whenComplete()).isCompletedExceptionally(); - } - - @Test - void factoryDuplicatorHasNoSizeCapAcrossManyDuplicates() { - // The factory duplicator never accumulates signal length, so no ContentTooLargeException - // is possible regardless of body size. This asserts the factory path is unaffected by the - // int cap that limits DefaultStreamMessageDuplicator (verified separately in that class's - // own tests). Here we simply confirm many large-"reported"-length duplications succeed. - final HttpRequest original = HttpRequest.of(HttpMethod.POST, "/orig"); - final Supplier factory = () -> HttpRequest.of(HttpMethod.POST, "/orig"); - final RequestFactoryHttpRequestDuplicator dup = - new RequestFactoryHttpRequestDuplicator(original, factory); - - // First (original) + several factory-produced duplicates, none throw. - assertThat(dup.duplicate()).isNotNull(); - for (int i = 0; i < 5; i++) { - assertThat(dup.duplicate()).isNotNull(); - } - } -} diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java new file mode 100644 index 00000000000..0f0102be9b8 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java @@ -0,0 +1,118 @@ +/* + * Copyright 2026 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.common; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpObject; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.stream.StreamMessage; + +class ReproducibleHttpRequestDuplicatorTest { + + private static final RequestHeaders HEADERS = RequestHeaders.of(HttpMethod.POST, "/upload"); + + @Test + void everyDuplicateProducesAFreshBody() { + final AtomicInteger calls = new AtomicInteger(); + final Supplier> factory = () -> { + calls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("body")); + }; + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + // Even the first duplicate() invokes the factory — the caller's request is never reused. + final HttpRequest first = dup.duplicate(); + assertThat(calls).hasValue(1); + final HttpRequest second = dup.duplicate(); + assertThat(calls).hasValue(2); + assertThat(first).isNotSameAs(second); + assertThat(first.headers().method()).isEqualTo(HttpMethod.POST); + } + + @Test + void duplicateWithHeadersOverridesHeaders() { + final Supplier> factory = + () -> StreamMessage.of(HttpData.ofUtf8("body")); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + final RequestHeaders overridden = RequestHeaders.of(HttpMethod.POST, "/upload", "x-attempt", "1"); + final HttpRequest req = dup.duplicate(overridden); + assertThat(req.headers().get("x-attempt")).isEqualTo("1"); + } + + @Test + void factoryThrowingPropagates() { + final Supplier> factory = () -> { + throw new IllegalStateException("cannot reproduce body"); + }; + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + // Fail fast: the exception propagates so the client can terminate the request. + assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class) + .hasMessageContaining("cannot reproduce body"); + } + + @Test + void factoryReturningNullThrows() { + final Supplier> factory = () -> null; + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + assertThatThrownBy(dup::duplicate).isInstanceOf(NullPointerException.class); + } + + @Test + void duplicateAfterCloseThrows() { + final Supplier> factory = + () -> StreamMessage.of(HttpData.ofUtf8("body")); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + dup.duplicate(); + dup.close(); + // StreamMessageDuplicator contract: duplicate() after close() must raise IllegalStateException. + assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class); + } + + @Test + void abortReleasesLastProducedUnsubscribedRequest() { + final Supplier> factory = + () -> StreamMessage.of(HttpData.ofUtf8("body")); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + final HttpRequest produced = dup.duplicate(); + // The produced request was never subscribed (e.g. endpoint selection threw); abort() must + // tear it down so its body is not leaked. + final RuntimeException cause = new RuntimeException("cleanup"); + dup.abort(cause); + assertThat(produced.whenComplete()).isCompletedExceptionally(); + } +} From 866dc10318c43504140e3e06027bc6c02549844c Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Thu, 9 Jul 2026 18:52:38 +0000 Subject: [PATCH 07/19] refactor(client): collapse duplicate toDuplicator overloads in ReproducibleHttpRequest The single-arg toDuplicator now delegates to the two-arg sink; both returned an identical duplicator. Merges the explanatory comments into one. No behavioral change. Co-authored-by: Isaac --- .../linecorp/armeria/common/ReproducibleHttpRequest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java index 4794fd12dc9..c9378ee6ca6 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -90,15 +90,14 @@ public CompletableFuture aggregate(AggregationOptions opt @Override public HttpRequestDuplicator toDuplicator(EventExecutor executor) { - // Ignore the executor: the reproducible duplicator never buffers, so it needs no subscriber - // executor. Every duplicate() obtains a fresh body from the factory. - return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); + return toDuplicator(executor, 0); } @Override public HttpRequestDuplicator toDuplicator(EventExecutor executor, long maxRequestLength) { - // maxRequestLength does not apply: this duplicator accumulates nothing, so there is no - // buffered length to cap. Each attempt streams a fresh body straight from the factory. + // Neither argument applies: the reproducible duplicator never buffers, so it needs no + // subscriber executor and has no accumulated length to cap. Each attempt streams a fresh + // body straight from the factory. return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); } } From 705aaf56632a6f1b2032d3c6b4159d8d46a8be0d Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Thu, 9 Jul 2026 19:01:58 +0000 Subject: [PATCH 08/19] docs+test: address CodeRabbit review on reproducible request bodies - HttpRequest.reproducible javadoc: declare the `path` local used in the example so the snippet is self-contained and compilable. - ReproducibleHttpRequestClientTest.stackedRetryAndRedirect: assert bodyCalls so the body-regeneration count through the retry+redirect chain is actually verified. Co-authored-by: Isaac --- core/src/main/java/com/linecorp/armeria/common/HttpRequest.java | 1 + .../armeria/client/ReproducibleHttpRequestClientTest.java | 2 ++ 2 files changed, 3 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index 689032af822..154f28106d2 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -368,6 +368,7 @@ static HttpRequest of(RequestHeaders headers, * opens a fresh body {@link StreamMessage} on demand: * *

{@code
+     * final Path path = Paths.get("/tmp/large-upload.bin");
      * final RequestHeaders headers =
      *         RequestHeaders.of(HttpMethod.POST, "/upload",
      *                           HttpHeaderNames.CONTENT_TYPE, "application/octet-stream");
diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java
index c31e0e51e2e..b2664753898 100644
--- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java
+++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java
@@ -239,5 +239,7 @@ void stackedRetryAndRedirect() {
 
         assertThat(res.status()).isEqualTo(HttpStatus.OK);
         assertThat(res.contentUtf8()).isEqualTo("redir-body");
+        // Body regenerated for the initial request and the redirected hop.
+        assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2);
     }
 }

From 57ea25dabc81e92ea53a349a026e11ac225d6d0a Mon Sep 17 00:00:00 2001
From: Yizhou Feng 
Date: Wed, 15 Jul 2026 06:42:45 +0000
Subject: [PATCH 09/19] refactor(client): track all outstanding duplicates and
 guard abort races

Address maintainer review (jrhee17, minwoox) on PR #6841:

- Match DefaultStreamMessageDuplicator semantics: every request produced by
  duplicate() stays active until it completes on its own, for as long as the
  duplicator is not aborted. Replace single last-produced tracking with a
  Set of children. close() prevents further duplication but leaves
  outstanding requests streaming; abort()/abort(cause) tears down every
  outstanding request. This also supports multiple concurrent outstanding
  requests (e.g. future hedging), not just the most recent one.

- Fix the abort-from-any-thread leak: abort() may run on the event loop while
  the caller thread is mid-duplicate(). Guard all state transitions with the
  instance lock so a concurrently produced request cannot be missed and leaked;
  a request produced after close/abort is aborted immediately and duplicate()
  then throws instead of returning an untearable request.

Co-authored-by: Isaac
---
 .../ReproducibleHttpRequestDuplicator.java    | 123 ++++++++++++------
 ...ReproducibleHttpRequestDuplicatorTest.java |  44 ++++++-
 2 files changed, 120 insertions(+), 47 deletions(-)

diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java
index 875b3a0739c..052802a0b51 100644
--- a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java
+++ b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java
@@ -18,6 +18,11 @@
 
 import static java.util.Objects.requireNonNull;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.List;
+import java.util.Set;
 import java.util.function.Supplier;
 
 import com.linecorp.armeria.common.HttpObject;
@@ -34,22 +39,31 @@
  * on the wire. This avoids the ~2 GiB {@code int} size limit and the memory cost of
  * {@code DefaultStreamMessageDuplicator}, which buffers the whole body for replay.
  *
- * 

{@code RetryingClient} and {@code RedirectingClient} drive this duplicator strictly - * sequentially: at most one produced request is outstanding at a time, and each access - * happens-after the previous one via the async response future. The duplicator tracks the single - * last-produced request so that {@link #abort(Throwable)} can tear it down if it was produced but - * never subscribed (e.g. endpoint selection threw before the wire took ownership). The fields are - * {@code volatile} for safe publication across the caller-thread → event-loop hand-off, matching how - * the clients drive it. + *

The lifecycle matches {@link com.linecorp.armeria.common.stream.StreamMessageDuplicator}: every + * request produced by {@link #duplicate(RequestHeaders)} stays active until it completes on its own, + * for as long as this duplicator is not aborted. {@link #close()} only prevents further duplication; + * it leaves outstanding requests streaming. {@link #abort(Throwable)} closes the duplicator and aborts + * every outstanding request so their bodies (e.g. open files) are released. This also lets multiple + * produced requests be outstanding concurrently (e.g. for hedging), not just the most recent one. + * + *

All state transitions are guarded by the instance lock so that {@link #abort(Throwable)}, which + * may be invoked from any thread (e.g. a response timeout on the event loop while the caller thread is + * mid-{@link #duplicate(RequestHeaders)}), cannot miss a request that is being produced concurrently + * and leak it. A request produced after the duplicator is closed or aborted is aborted immediately by + * {@code duplicate}, and {@code duplicate} then throws instead of returning a request that would never + * be torn down. */ public final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { private final RequestHeaders headers; private final Supplier> bodyFactory; + private final Set children = + Collections.newSetFromMap(new IdentityHashMap<>()); + + private boolean closed; @Nullable - private volatile HttpRequest lastProduced; - private volatile boolean closed; + private Throwable abortCause; public ReproducibleHttpRequestDuplicator( RequestHeaders headers, @@ -71,60 +85,85 @@ public HttpRequest duplicate() { @Override public HttpRequest duplicate(RequestHeaders newHeaders) { requireNonNull(newHeaders, "newHeaders"); - if (closed) { - throw new IllegalStateException("duplicator is closed or aborted."); - } - // A produced-but-unsubscribed request from a previous attempt (e.g. endpoint selection threw - // before the wire subscribed) would otherwise be lost when we overwrite lastProduced; abort it - // so its body is released. Aborting an already-subscribed stream is a no-op. - final HttpRequest previous = lastProduced; - if (previous != null) { - previous.abort(); - } - - // Fail fast on a broken factory: propagate to the caller (RetryingClient / RedirectingClient), - // which completes the response exceptionally instead of re-judging an aborted request and - // wasting the retry budget. + // Produce the body outside the lock: the factory is user code (it may open a file or block), + // and holding the lock across it would widen the window during which a concurrent abort() is + // stalled. Fail fast on a broken factory by propagating to the caller (RetryingClient / + // RedirectingClient), which completes the response exceptionally instead of re-judging an + // aborted request and wasting the retry budget. final StreamMessage body = requireNonNull(bodyFactory.get(), "bodyFactory.get() returned null."); final HttpRequest produced = HttpRequest.of(newHeaders, body); - lastProduced = produced; - return produced; + + final Throwable abortCause; + synchronized (this) { + if (!closed) { + children.add(produced); + // Drop the request from the tracked set once it finishes so the set does not grow + // unbounded across a long retry/redirect chain. Registered while holding the lock so a + // concurrent abort() sees a consistent snapshot. + produced.whenComplete().handle((unused, cause) -> { + synchronized (this) { + children.remove(produced); + } + return null; + }); + return produced; + } + // Closed or aborted while we were producing: tear down the just-produced request so its + // body is released, then report the closed state to the caller. + abortCause = this.abortCause; + } + abort(produced, abortCause); + throw new IllegalStateException("duplicator is closed or aborted."); } @Override public void close() { - // Let the last-produced request finish: on the success path the wire owns it and is still - // streaming, and the StreamMessageDuplicator contract says close() must not abort issued - // duplicates. If it was never subscribed, it is completed/aborted by its own consumer or by a - // subsequent abort(); we intentionally do not abort it here. - closed = true; + // Prevent further duplication but leave outstanding requests streaming: on the success path the + // wire owns them and is still streaming, and the StreamMessageDuplicator contract says close() + // must not abort issued duplicates. + synchronized (this) { + closed = true; + } } @Override public void abort() { - abortLastProduced(null); + abortAll(null); } @Override public void abort(Throwable cause) { - abortLastProduced(requireNonNull(cause, "cause")); + abortAll(requireNonNull(cause, "cause")); } - private void abortLastProduced(@Nullable Throwable cause) { - closed = true; - final HttpRequest lastProduced = this.lastProduced; - if (lastProduced == null) { - return; + private void abortAll(@Nullable Throwable cause) { + final List toAbort; + synchronized (this) { + closed = true; + if (cause != null) { + // Remember the cause so a request produced by a racing duplicate() is aborted with it. + abortCause = cause; + } + toAbort = new ArrayList<>(children); + children.clear(); + } + // Abort outside the lock: whenComplete callbacks fire synchronously during abort() and re-enter + // the lock to remove the child, so holding it here would extend the critical section across every + // child's teardown and contend needlessly. The children set was already cleared above. + for (HttpRequest child : toAbort) { + abort(child, cause); } - this.lastProduced = null; - // If the wire already owns the subscription, abort() on an already-subscribed stream is a - // no-op. Otherwise this releases the produced-but-unsubscribed body (e.g. an open file). + } + + private static void abort(HttpRequest request, @Nullable Throwable cause) { + // Aborting an already-subscribed stream is a no-op; otherwise this releases a + // produced-but-unsubscribed body (e.g. an open file). if (cause != null) { - lastProduced.abort(cause); + request.abort(cause); } else { - lastProduced.abort(); + request.abort(); } } } diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java index 0f0102be9b8..63ce32dda0d 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java @@ -102,17 +102,51 @@ void duplicateAfterCloseThrows() { } @Test - void abortReleasesLastProducedUnsubscribedRequest() { + void abortReleasesAllOutstandingUnsubscribedRequests() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); final ReproducibleHttpRequestDuplicator dup = new ReproducibleHttpRequestDuplicator(HEADERS, factory); - final HttpRequest produced = dup.duplicate(); - // The produced request was never subscribed (e.g. endpoint selection threw); abort() must - // tear it down so its body is not leaked. + // Multiple produced requests may be outstanding at once (e.g. hedging); abort() must tear + // down every one of them, not just the most recently produced, so no body is leaked. + final HttpRequest first = dup.duplicate(); + final HttpRequest second = dup.duplicate(); final RuntimeException cause = new RuntimeException("cleanup"); dup.abort(cause); - assertThat(produced.whenComplete()).isCompletedExceptionally(); + assertThat(first.whenComplete()).isCompletedExceptionally(); + assertThat(second.whenComplete()).isCompletedExceptionally(); + } + + @Test + void closeLeavesOutstandingRequestsActive() { + final Supplier> factory = + () -> StreamMessage.of(HttpData.ofUtf8("body")); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + final HttpRequest produced = dup.duplicate(); + // StreamMessageDuplicator contract: close() prevents further duplication but must not abort + // requests that were already produced — they keep streaming until they complete on their own. + dup.close(); + assertThat(produced.whenComplete()).isNotDone(); + } + + @Test + void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { + final Supplier> factory = + () -> StreamMessage.of(HttpData.ofUtf8("body")); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + // Draining a produced request completes it; it is then removed from the tracked set so the set + // does not grow unbounded and a later abort() leaves the already-completed request untouched. + final HttpRequest produced = dup.duplicate(); + produced.aggregate().join(); + assertThat(produced.whenComplete()).isCompleted(); + + dup.abort(new RuntimeException("cleanup")); + assertThat(produced.whenComplete()).isCompleted() + .isNotCompletedExceptionally(); } } From 80de3650b0998d5ae72659d73f58ca8fbbffe6bf Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 15 Jul 2026 06:42:57 +0000 Subject: [PATCH 10/19] docs+test: correct decorator ordering, cover multi-chunk bodies, tighten assert Address review findings on reproducible request bodies: - HttpRequest.reproducible Javadoc had the retry/redirect nesting backwards. In a WebClient, RedirectingClient wraps the user decorators (incl. RetryingClient), so RedirectingClient is the outermost duplicating decorator and replays reproducibly across redirect hops; an inner RetryingClient re-buffers when retrying within a single hop. Correct the direction. - ReproducibleHttpRequest Javadoc claimed the factory is "never called eagerly / invoked exactly once on subscription". abort() before any real subscriber subscribes an aborting subscriber, which runs the factory once. Document that abort-before-subscribe counts as the first subscription (factory runs at most once on the direct path), and note the cold-path single-arg subscribe drops SubscriptionOptions / the caller executor. - Add retryReproducesMultiChunkBodyAndTrailers: a genuinely multi-chunk body terminated by a trailer, asserting order and trailer survive reproduction on the re-sent attempt (previous tests only used a single chunk, no trailers). - Tighten factoryThrowingOnRetryFailsFast to assert the surfaced root cause is the factory's IllegalStateException, not merely some Exception. Co-authored-by: Isaac --- .../linecorp/armeria/common/HttpRequest.java | 11 ++-- .../common/ReproducibleHttpRequest.java | 24 +++++++-- .../ReproducibleHttpRequestClientTest.java | 54 ++++++++++++++++++- 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index 154f28106d2..30e65b23c25 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -387,10 +387,13 @@ static HttpRequest of(RequestHeaders headers, * {@link HttpHeaderNames#CONTENT_LENGTH} (if any), exactly as for any streaming * {@link HttpRequest}. * - *

Reproducible replay applies only to the outermost duplicating decorator (typically - * {@code RetryingClient}). Each attempt handed downstream is an ordinary {@link HttpRequest}; - * an inner decorator (e.g. a redirect within a single retry attempt) treats it as a normal - * request and may buffer it. It is honored for streaming requests + *

Reproducible replay applies only at the outermost duplicating decorator, which for a + * {@code WebClient} is {@code RedirectingClient} (it wraps the user-supplied decorators, including + * {@code RetryingClient}). Each attempt that the outermost decorator hands downstream is an + * ordinary {@link HttpRequest}, so an inner decorator treats it as a normal request and may buffer + * it: with both redirect and retry enabled, {@code RedirectingClient} replays reproducibly across + * redirect hops, but an inner {@code RetryingClient} re-buffers the body when retrying within a + * single hop. Reproducibility is honored for streaming requests * ({@link ExchangeType#isRequestStreaming()}); an aggregated exchange type buffers the body as * usual. * diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java index c9378ee6ca6..eda491340fd 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -33,11 +33,19 @@ * *

See {@link HttpRequest#reproducible(RequestHeaders, Supplier)} for details and usage. * - *

The body factory is invoked lazily: when this request is duplicated (the retry/redirect path), - * {@link #toDuplicator(EventExecutor, long)} returns a {@link ReproducibleHttpRequestDuplicator} that - * calls the factory once per attempt and this request's own delegate is never subscribed. When this - * request is subscribed directly (no retry/redirect decorator), the factory is invoked exactly once, - * on subscription, to produce the single body. Either way the factory is never called eagerly. + *

The body factory is invoked lazily: it is never called during construction. On the + * retry/redirect path {@link #toDuplicator(EventExecutor, long)} returns a + * {@link ReproducibleHttpRequestDuplicator} that calls the factory once per attempt, and this + * request's own delegate is never subscribed. When this request is instead consumed directly (no + * retry/redirect decorator), the factory is invoked once when the delegate is first subscribed to + * produce the single body. + * + *

"First subscription" here includes {@link #abort()}/{@link #abort(Throwable)} called before any + * real subscriber arrives: the underlying {@link StreamMessage} subscribes an aborting subscriber, + * which runs the factory once so the produced body can be aborted and released. This means aborting a + * directly-consumed request that was never sent still invokes the factory exactly once (it does not + * regenerate on a subsequent subscribe, because a stream permits only one subscription). The factory + * therefore runs at most once on the direct path. */ final class ReproducibleHttpRequest extends NonOverridableStreamMessageWrapper implements HttpRequest { @@ -73,6 +81,12 @@ private static StreamMessage lazyBody( } @SuppressWarnings("unchecked") final StreamMessage cast = (StreamMessage) body; + // This single-arg subscribe forwards neither the caller's SubscriptionOptions + // (WITH_POOLED_OBJECTS / NOTIFY_CANCELLATION) nor the requested executor to the produced + // body. That is acceptable only because this is a cold path: a reproducible request is + // meant to be driven by a retry/redirect decorator, which uses toDuplicator(...) and never + // subscribes this delegate. This branch is reached only when a reproducible request is + // consumed directly, with no such decorator. cast.subscribe(subscriber); }); } diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java index b2664753898..642ad8e7993 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -32,6 +32,7 @@ import com.linecorp.armeria.common.ExchangeType; import com.linecorp.armeria.common.HttpData; import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpHeaders; import com.linecorp.armeria.common.HttpMethod; import com.linecorp.armeria.common.HttpObject; import com.linecorp.armeria.common.HttpRequest; @@ -71,6 +72,20 @@ protected void configure(ServerBuilder sb) { req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, agg.contentUtf8()).toHttpResponse()))); + // Echoes the concatenated body plus the received trailer, failing the first attempt so the + // multi-chunk body is reproduced across a retry. + sb.service("/multi", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> { + final int hit = serverHits.incrementAndGet(); + if (hit == 1) { + return AggregatedHttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR) + .toHttpResponse(); + } + final String trailer = agg.trailers().get("x-trailer", ""); + return AggregatedHttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8() + '|' + trailer) + .toHttpResponse(); + }))); // 303 See Other: method rewritten to GET and body dropped. sb.service("/see-other", (ctx, req) -> HttpResponse.of( ResponseHeaders.of(HttpStatus.SEE_OTHER, HttpHeaderNames.LOCATION, "/target"))); @@ -135,6 +150,39 @@ void retryRegeneratesBody() { assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); } + @Test + void retryReproducesMultiChunkBodyAndTrailers() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/multi", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + // A genuinely multi-chunk body terminated by a trailer, the shape a streaming upload takes. + // Each attempt must reproduce every interior chunk in order plus the trailer. + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("a"), + HttpData.ofUtf8("b"), + HttpData.ofUtf8("c"), + HttpHeaders.of("x-trailer", "v")); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + // Concatenated interior chunks (in order) plus the reproduced trailer, on the re-sent attempt. + assertThat(res.contentUtf8()).isEqualTo("abc|v"); + assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); + assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + } + @Test void factoryThrowingOnRetryFailsFast() { final AtomicInteger bodyCalls = new AtomicInteger(); @@ -155,10 +203,14 @@ void factoryThrowingOnRetryFailsFast() { RetryRule.builder().onServerErrorStatus().thenBackoff())) .build(); + // The surfaced failure must carry the factory's own exception, not merely be "some Exception": + // a connection error, timeout, or unrelated bug would also satisfy isInstanceOf(Exception.class). assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join()) - .isInstanceOf(Exception.class); + .getRootCause() + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("cannot regenerate body"); // The factory is consulted twice: once for the initial body (via reproducible()), once for the // failing retry. It fails fast rather than looping through the whole retry budget. assertThat(bodyCalls).hasValue(2); From 8860934d3cbe23c8191b0196e1a3521215c4e545 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Wed, 15 Jul 2026 07:00:10 +0000 Subject: [PATCH 11/19] style: rename abort helper to satisfy OverloadMethodsDeclarationOrder The private static abort(HttpRequest, Throwable) helper was an overload of abort()/abort(Throwable) with abortAll() declared between them, tripping checkstyle's OverloadMethodsDeclarationOrder. Rename the helper to abortQuietly so it is no longer an overload; no behavior change. Co-authored-by: Isaac --- .../internal/common/ReproducibleHttpRequestDuplicator.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java index 052802a0b51..1bc636bd535 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java @@ -114,7 +114,7 @@ public HttpRequest duplicate(RequestHeaders newHeaders) { // body is released, then report the closed state to the caller. abortCause = this.abortCause; } - abort(produced, abortCause); + abortQuietly(produced, abortCause); throw new IllegalStateException("duplicator is closed or aborted."); } @@ -153,11 +153,11 @@ private void abortAll(@Nullable Throwable cause) { // the lock to remove the child, so holding it here would extend the critical section across every // child's teardown and contend needlessly. The children set was already cleared above. for (HttpRequest child : toAbort) { - abort(child, cause); + abortQuietly(child, cause); } } - private static void abort(HttpRequest request, @Nullable Throwable cause) { + private static void abortQuietly(HttpRequest request, @Nullable Throwable cause) { // Aborting an already-subscribed stream is a no-op; otherwise this releases a // produced-but-unsubscribed body (e.g. an open file). if (cause != null) { From fa94f5c5c5dc65eb8fe20dea61730eb100e69950 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Thu, 16 Jul 2026 03:07:52 +0000 Subject: [PATCH 12/19] fix(client): keep reproducible body across header rewrites; harden docs+tests Addresses code-review findings on the reproducible-request-bodies work: - ReproducibleHttpRequest now overrides withHeaders() to rebind the same body factory to the new headers. Previously a header rewrite (e.g. a base-URI path prefix applied by DefaultWebClient, or a decorator overriding the path) wrapped the request in HeaderOverridingHttpRequest, which does not override toDuplicator and fell back to the buffering DefaultStreamMessageDuplicator -- silently reintroducing the ~2 GiB ContentTooLargeException this request type exists to avoid. - Corrects the HttpRequest.reproducible javadoc: RedirectingClient is not always the outermost duplicating decorator (redirects are disabled by default), so with retries only, RetryingClient is outermost and replays reproducibly. Also documents that each factory invocation must produce an equivalent body (not merely the same length), since the fixed content-length is reused unvalidated, and documents throwing/null factory fail-fast semantics. - Tests: add direct-consume coverage (normal / throwing / null factory via a plain WebClient with no decorator), a base-path-prefix regression test that fails without the withHeaders fix, and non-buffering proofs (toDuplicator ignores maxRequestLength; large body streams without accumulation). Tighten deterministic factory-invocation assertions from >= 2 to exactly 2 so an over-regeneration bug is caught rather than masked. Co-authored-by: Isaac --- .../linecorp/armeria/common/HttpRequest.java | 42 +++-- .../common/ReproducibleHttpRequest.java | 17 ++ .../ReproducibleHttpRequestClientTest.java | 161 +++++++++++++++++- ...ReproducibleHttpRequestDuplicatorTest.java | 18 ++ 4 files changed, 217 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index 30e65b23c25..affda30f859 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -383,23 +383,39 @@ static HttpRequest of(RequestHeaders headers, *

The {@code bodyFactory} is invoked once per attempt (initial request, retry attempt, or * redirect hop). The fixed {@code headers} are reused for every attempt; the factory regenerates * only the body {@link StreamMessage}, so the request method and headers cannot drift between - * attempts. The body each factory invocation produces must match the declared - * {@link HttpHeaderNames#CONTENT_LENGTH} (if any), exactly as for any streaming - * {@link HttpRequest}. - * - *

Reproducible replay applies only at the outermost duplicating decorator, which for a - * {@code WebClient} is {@code RedirectingClient} (it wraps the user-supplied decorators, including - * {@code RetryingClient}). Each attempt that the outermost decorator hands downstream is an - * ordinary {@link HttpRequest}, so an inner decorator treats it as a normal request and may buffer - * it: with both redirect and retry enabled, {@code RedirectingClient} replays reproducibly across - * redirect hops, but an inner {@code RetryingClient} re-buffers the body when retrying within a - * single hop. Reproducibility is honored for streaming requests + * attempts. Every invocation must produce an equivalent body — the same bytes and + * trailers, not merely the same length — because the fixed {@code headers} (including any declared + * {@link HttpHeaderNames#CONTENT_LENGTH}) are reused verbatim on every attempt and are not + * re-validated against the regenerated body. A factory whose output varies across invocations + * (e.g. it embeds a timestamp, or reads a file being mutated concurrently) silently sends + * different data on a retry or redirect, and a length mismatch against a declared + * {@code content-length} corrupts wire framing (the request stalls or the next message is + * garbled) with no error surfaced. + * + *

Reproducible replay applies only at the outermost duplicating decorator; each attempt + * it hands downstream is an ordinary {@link HttpRequest}, so any inner duplicating decorator treats + * it as a normal request and buffers it. Which decorator is outermost depends on configuration: + *

    + *
  • With retries only (the common case; redirects are disabled by default), + * {@code RetryingClient} is the outermost decorator and replays reproducibly across every + * retry.
  • + *
  • With {@code followRedirects()} enabled, the built-in {@code RedirectingClient} wraps the + * user-supplied decorators (including {@code RetryingClient}), so it replays reproducibly + * across redirect hops, but an inner {@code RetryingClient} re-buffers the body when retrying + * within a single hop — reintroducing the ~2 GiB limit for that retry. If you need + * non-buffering retries of a very large body, avoid stacking {@code RetryingClient} beneath + * {@code RedirectingClient}.
  • + *
+ * Reproducibility is honored for streaming requests * ({@link ExchangeType#isRequestStreaming()}); an aggregated exchange type buffers the body as * usual. * * @param headers the fixed {@link RequestHeaders} reused for every attempt - * @param bodyFactory produces a fresh body {@link StreamMessage} for each attempt; it must not - * return {@code null} + * @param bodyFactory produces a fresh body {@link StreamMessage} for each attempt, each equivalent + * to the first; it must not return {@code null}. If an invocation throws or + * returns {@code null}, that attempt fails with the thrown cause (or a + * {@link NullPointerException}) and the failure is propagated to the caller + * without exhausting the remaining retry budget. */ @UnstableApi static HttpRequest reproducible( diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java index eda491340fd..7ddc436b445 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -16,6 +16,8 @@ package com.linecorp.armeria.common; +import static java.util.Objects.requireNonNull; + import java.util.concurrent.CompletableFuture; import java.util.function.Supplier; @@ -96,6 +98,21 @@ public RequestHeaders headers() { return headers; } + @Override + public HttpRequest withHeaders(RequestHeaders newHeaders) { + requireNonNull(newHeaders, "newHeaders"); + if (headers == newHeaders) { + return this; + } + // Preserve reproducibility across a header rewrite (e.g. a base-URI path prefix applied by + // DefaultWebClient, or a redirect/retry decorator overriding the path). The default + // HttpRequest.withHeaders wraps this in a HeaderOverridingHttpRequest, which does not override + // toDuplicator and would therefore fall back to the buffering DefaultHttpRequestDuplicator, + // reintroducing the ~2 GiB size limit this request type exists to avoid. Rebinding the same + // factory to the new headers keeps the non-buffering toDuplicator path. + return new ReproducibleHttpRequest(newHeaders, bodyFactory); + } + @SuppressWarnings("unchecked") @Override public CompletableFuture aggregate(AggregationOptions options) { diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java index 642ad8e7993..abb0c636174 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -28,7 +28,9 @@ import com.linecorp.armeria.client.retry.RetryRule; import com.linecorp.armeria.client.retry.RetryingClient; +import com.linecorp.armeria.common.AggregatedHttpRequest; import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.CommonPools; import com.linecorp.armeria.common.ExchangeType; import com.linecorp.armeria.common.HttpData; import com.linecorp.armeria.common.HttpHeaderNames; @@ -36,6 +38,7 @@ import com.linecorp.armeria.common.HttpMethod; import com.linecorp.armeria.common.HttpObject; import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpRequestDuplicator; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.MediaType; @@ -45,6 +48,8 @@ import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.testing.junit5.server.ServerExtension; +import io.netty.util.concurrent.EventExecutor; + class ReproducibleHttpRequestClientTest { private static final AtomicInteger serverHits = new AtomicInteger(); @@ -93,6 +98,24 @@ protected void configure(ServerBuilder sb) { req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, req.method() + ":" + agg.contentUtf8()).toHttpResponse()))); + // Plain echo that always succeeds, for exercising the request without any retry/redirect + // decorator (the direct-consume path) and for a base-path-prefixed client. + sb.service("/echo", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> AggregatedHttpResponse.of( + HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8()).toHttpResponse()))); + // Same fail-once-then-echo behavior as /upload, reached only when a base-URI path prefix + // ("/api") rewrites the request path — the scenario that must keep the reproducible body. + sb.service("/api/upload", (ctx, req) -> HttpResponse.of( + req.aggregate().thenApply(agg -> { + final int hit = serverHits.incrementAndGet(); + if (hit == 1) { + return AggregatedHttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR) + .toHttpResponse(); + } + return AggregatedHttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, + agg.contentUtf8()).toHttpResponse(); + }))); } }; @@ -145,9 +168,12 @@ void retryRegeneratesBody() { assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("hello-body"); - assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); + // The server fails exactly once, so the attempt count is deterministic: initial + one retry. + // Assert exactly 2 so a double-subscription bug that regenerates the body 3+ times (leaking a + // fresh factory resource per attempt) is caught rather than masked by a >= assertion. + assertThat(serverHits).hasValue(2); // Body regenerated for the initial attempt and the retry. - assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + assertThat(bodyCalls).hasValue(2); } @Test @@ -179,8 +205,9 @@ void retryReproducesMultiChunkBodyAndTrailers() { assertThat(res.status()).isEqualTo(HttpStatus.OK); // Concatenated interior chunks (in order) plus the reproduced trailer, on the re-sent attempt. assertThat(res.contentUtf8()).isEqualTo("abc|v"); - assertThat(serverHits).hasValueGreaterThanOrEqualTo(2); - assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + // Deterministic: initial + one retry. Exact assertion guards against over-regeneration. + assertThat(serverHits).hasValue(2); + assertThat(bodyCalls).hasValue(2); } @Test @@ -238,8 +265,9 @@ void followsRedirectRegeneratingBody() { assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("redir-body"); - // Body regenerated for the initial request and the redirected hop. - assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + // Deterministic: initial request + one redirect hop (no server error, so no retry). Exact + // assertion guards against over-regeneration. + assertThat(bodyCalls).hasValue(2); } @Test @@ -291,7 +319,124 @@ void stackedRetryAndRedirect() { assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("redir-body"); - // Body regenerated for the initial request and the redirected hop. - assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + // Deterministic: initial request + one redirect hop (no server error, so no retry). Exact + // assertion guards against over-regeneration. + assertThat(bodyCalls).hasValue(2); + } + + @Test + void directConsumeWithoutDecoratorSendsBodyOnce() { + // No retry/redirect decorator, so the request is consumed directly via its lazyBody delegate + // (never through toDuplicator). This path is otherwise unexercised — every other test drives a + // decorator. The factory must be invoked exactly once and the body delivered intact. + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/echo", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("direct-body")); + }; + + final WebClient client = WebClient.of(server.httpUri()); + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("direct-body"); + assertThat(bodyCalls).hasValue(1); + } + + @Test + void directConsumeSurfacesThrowingFactory() { + // On the direct path, a throwing factory must surface its own cause to the subscriber rather + // than hang or swallow the error. + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/echo", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + throw new IllegalStateException("cannot produce body"); + }; + + final WebClient client = WebClient.of(server.httpUri()); + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + streamingOptions()) + .aggregate().join()) + .getRootCause() + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("cannot produce body"); + } + + @Test + void directConsumeSurfacesNullFactory() { + // On the direct path, a factory returning null must surface an NPE, matching the duplicator + // path's null handling. + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/echo", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> null; + + final WebClient client = WebClient.of(server.httpUri()); + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + streamingOptions()) + .aggregate().join()) + .getRootCause() + .isInstanceOf(NullPointerException.class); + } + + @Test + void toDuplicatorIgnoresMaxRequestLength() { + // The reproducible duplicator never buffers, so it must ignore the maxRequestLength cap that a + // buffering DefaultStreamMessageDuplicator would enforce. A body far larger than a tiny cap must + // still stream to completion; a regression that fell back to a buffering duplicator would throw + // ContentTooLargeException here. + final byte[] large = new byte[64 * 1024]; + final Supplier> bodyFactory = + () -> StreamMessage.of(HttpData.wrap(large)); + final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/echo"); + + final EventExecutor executor = CommonPools.workerGroup().next(); + final HttpRequestDuplicator duplicator = + HttpRequest.reproducible(headers, bodyFactory).toDuplicator(executor, 8); + + final AggregatedHttpRequest produced = duplicator.duplicate().aggregate().join(); + assertThat(produced.content().length()).isEqualTo(large.length); + duplicator.close(); + } + + @Test + void basePathPrefixRemainsReproducible() { + // A WebClient built with a base-URI path prefix rewrites the request path via + // req.withHeaders(...). If ReproducibleHttpRequest did not override withHeaders, the rewritten + // request would be a plain HeaderOverridingHttpRequest whose toDuplicator falls back to the + // buffering DefaultStreamMessageDuplicator — silently reintroducing the ~2 GiB limit. This test + // pins that the rewritten request still regenerates its body per attempt (non-buffering path). + final AtomicInteger bodyCalls = new AtomicInteger(); + // Header path is "/upload"; the base URI prefix "/api" makes the effective path "/api/upload", + // forcing a path rewrite. Route the server so /api/upload retries once like /upload does. + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/upload", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("prefixed-body")); + }; + + final WebClient client = + WebClient.builder(server.httpUri() + "/api") + .decorator(RetryingClient.newDecorator( + RetryRule.builder().onServerErrorStatus().thenBackoff())) + .build(); + + final AggregatedHttpResponse res = + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + .aggregate().join(); + + assertThat(res.status()).isEqualTo(HttpStatus.OK); + assertThat(res.contentUtf8()).isEqualTo("prefixed-body"); + // Regenerated for the initial attempt and the retry — proving the path-rewritten request kept + // the reproducible (non-buffering) duplicator rather than falling back to buffering. + assertThat(bodyCalls).hasValue(2); } } diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java index 63ce32dda0d..5c85496f547 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java @@ -132,6 +132,24 @@ void closeLeavesOutstandingRequestsActive() { assertThat(produced.whenComplete()).isNotDone(); } + @Test + void streamsBodyWithoutAccumulatingIt() { + // The whole point of the reproducible duplicator is to avoid buffering the body for replay (and + // thus the ~2 GiB int32 cap of DefaultStreamMessageDuplicator). A produced request must stream + // its body straight through rather than accumulate it; here we simply assert a large body is + // delivered intact. See ReproducibleHttpRequestClientTest#toDuplicatorIgnoresMaxRequestLength + // for the companion proof that the maxRequestLength cap is not applied. + final byte[] large = new byte[64 * 1024]; + final Supplier> factory = + () -> StreamMessage.of(HttpData.wrap(large)); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + final HttpRequest produced = dup.duplicate(); + final int received = produced.aggregate().join().content().length(); + assertThat(received).isEqualTo(large.length); + } + @Test void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { final Supplier> factory = From f30c82c3a1b6869113c08f2d6a2fecacef386899 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 4 Aug 2026 01:26:24 +0000 Subject: [PATCH 13/19] refactor(common): move ReproducibleHttpRequestDuplicator into common, package-private The duplicator was public only to be reachable across the internal.common -> common package boundary from ReproducibleHttpRequest#toDuplicator. It has no external consumers, so co-locating it with its sole caller in com.linecorp.armeria.common lets it (and its constructor) become package-private, matching the sibling DefaultHttpRequestDuplicator. No behavior change. Co-authored-by: Isaac --- .../armeria/common/ReproducibleHttpRequest.java | 1 - .../common/ReproducibleHttpRequestDuplicator.java | 10 +++------- .../common/ReproducibleHttpRequestDuplicatorTest.java | 7 +------ 3 files changed, 4 insertions(+), 14 deletions(-) rename core/src/main/java/com/linecorp/armeria/{internal => }/common/ReproducibleHttpRequestDuplicator.java (94%) rename core/src/test/java/com/linecorp/armeria/{internal => }/common/ReproducibleHttpRequestDuplicatorTest.java (96%) diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java index 7ddc436b445..c09a06cefc2 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -24,7 +24,6 @@ import org.reactivestreams.Publisher; import com.linecorp.armeria.common.stream.StreamMessage; -import com.linecorp.armeria.internal.common.ReproducibleHttpRequestDuplicator; import com.linecorp.armeria.internal.common.stream.NonOverridableStreamMessageWrapper; import io.netty.util.concurrent.EventExecutor; diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java similarity index 94% rename from core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java rename to core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java index 1bc636bd535..896317b161d 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicator.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.internal.common; +package com.linecorp.armeria.common; import static java.util.Objects.requireNonNull; @@ -25,10 +25,6 @@ import java.util.Set; import java.util.function.Supplier; -import com.linecorp.armeria.common.HttpObject; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.HttpRequestDuplicator; -import com.linecorp.armeria.common.RequestHeaders; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.stream.StreamMessage; @@ -53,7 +49,7 @@ * {@code duplicate}, and {@code duplicate} then throws instead of returning a request that would never * be torn down. */ -public final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { +final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { private final RequestHeaders headers; private final Supplier> bodyFactory; @@ -65,7 +61,7 @@ public final class ReproducibleHttpRequestDuplicator implements HttpRequestDupli @Nullable private Throwable abortCause; - public ReproducibleHttpRequestDuplicator( + ReproducibleHttpRequestDuplicator( RequestHeaders headers, Supplier> bodyFactory) { this.headers = requireNonNull(headers, "headers"); diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java similarity index 96% rename from core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java rename to core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java index 5c85496f547..af6f31c6205 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.internal.common; +package com.linecorp.armeria.common; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -24,11 +24,6 @@ import org.junit.jupiter.api.Test; -import com.linecorp.armeria.common.HttpData; -import com.linecorp.armeria.common.HttpMethod; -import com.linecorp.armeria.common.HttpObject; -import com.linecorp.armeria.common.HttpRequest; -import com.linecorp.armeria.common.RequestHeaders; import com.linecorp.armeria.common.stream.StreamMessage; class ReproducibleHttpRequestDuplicatorTest { From 1286461249864ea0a75884fc7faf50c04427a308 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 4 Aug 2026 01:26:49 +0000 Subject: [PATCH 14/19] test(client): assert factory call count for direct consume, abort, and 303 Lock in the reproducibility contracts that were previously only asserted in prose: - factoryInvokedOnceOnDirectConsumption: direct consumption (no retry/redirect decorator) subscribes the lazy delegate and runs the factory exactly once. - abortBeforeSubscriptionInvokesFactoryOnce: aborting a never-sent request runs the factory exactly once to release the produced body, and never regenerates. - seeOtherRedirectDropsBody now asserts the factory is called exactly once: the 303 hop drops the body (aborting the duplicator) instead of regenerating it, addressing the missing-assertion review note. Co-authored-by: Isaac --- .../ReproducibleHttpRequestClientTest.java | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java index abb0c636174..4026d7c31a9 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.concurrent.CompletionException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; @@ -145,6 +146,41 @@ void factoryNotInvokedEagerly() { req.abort(); } + @Test + void factoryInvokedOnceOnDirectConsumption() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/upload"); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + }; + + // Consuming the request directly (no retry/redirect decorator) subscribes the lazy delegate, + // which invokes the factory exactly once to produce the single body. + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + assertThat(req.aggregate().join().contentUtf8()).isEqualTo("hello-body"); + assertThat(bodyCalls).hasValue(1); + } + + @Test + void abortBeforeSubscriptionInvokesFactoryOnce() { + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/upload"); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("hello-body")); + }; + + // Aborting a directly-consumed request that was never sent subscribes an aborting subscriber, + // which runs the factory once so the produced body can be released. It does not regenerate on a + // later subscribe (a stream permits only one subscription), so the factory runs at most once. + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + req.abort(); + // abort() propagates asynchronously; join() blocks until completion before we assert the count. + assertThatThrownBy(() -> req.whenComplete().join()).isInstanceOf(CompletionException.class); + assertThat(bodyCalls).hasValue(1); + } + @Test void retryRegeneratesBody() { final AtomicInteger bodyCalls = new AtomicInteger(); @@ -272,11 +308,14 @@ void followsRedirectRegeneratingBody() { @Test void seeOtherRedirectDropsBody() { + final AtomicInteger bodyCalls = new AtomicInteger(); final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/see-other", HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); - final Supplier> bodyFactory = - () -> StreamMessage.of(HttpData.ofUtf8("see-other-body")); + final Supplier> bodyFactory = () -> { + bodyCalls.incrementAndGet(); + return StreamMessage.of(HttpData.ofUtf8("see-other-body")); + }; final WebClient client = WebClient.builder(server.httpUri()) @@ -291,6 +330,9 @@ void seeOtherRedirectDropsBody() { // and must not throw. assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("GET:"); + // The factory is invoked once for the initial POST attempt; the SEE_OTHER hop drops the body + // (aborting the duplicator) instead of regenerating it, so it is never called again. + assertThat(bodyCalls).hasValue(1); } @Test From 1f8894580949c41710f8873fe325b80692afa3f5 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 4 Aug 2026 06:02:37 +0000 Subject: [PATCH 15/19] test: cover abort-race teardown and redirect-hop factory failure; tighten retry assert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code-review findings (all test-only, no production change): - duplicateAfterAbortTearsDownProducedBodyWithCause: exercises the abortCause propagation branch of the duplicate()/abort(cause) race — the just-produced body is torn down with the remembered cause and duplicate() throws. Previously only the close() variant (abortCause == null) was covered. - factoryThrowingOnRedirectHopFailsFast: a factory that throws while regenerating the body for a redirect hop fails the request fast via RedirectingClient's try/catch, mirroring the existing RetryingClient coverage. - retriesAfterFirstAttemptBodyFailsMidStream: assert exactly 2 factory calls instead of >= 2, so an over-regeneration bug is caught rather than masked. Co-authored-by: Isaac --- .../ReproducibleHttpRequestClientTest.java | 30 +++++++++++++++++++ .../ReproducibleHttpRequestRetryTest.java | 6 ++-- ...ReproducibleHttpRequestDuplicatorTest.java | 29 ++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java index 4026d7c31a9..e1235adb996 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -366,6 +366,36 @@ void stackedRetryAndRedirect() { assertThat(bodyCalls).hasValue(2); } + @Test + void factoryThrowingOnRedirectHopFailsFast() { + // RedirectingClient calls reqDuplicator.duplicate() inside its try/catch, so a factory that + // throws while regenerating the body for a redirect hop fails the request via handleException + // rather than escaping or hanging. Mirror of factoryThrowingOnRetryFailsFast for the redirect + // path: the factory succeeds on the initial /first POST but throws on the /second hop. + final AtomicInteger bodyCalls = new AtomicInteger(); + final RequestHeaders headers = + RequestHeaders.of(HttpMethod.POST, "/first", + HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); + final Supplier> bodyFactory = () -> { + if (bodyCalls.incrementAndGet() == 1) { + return StreamMessage.of(HttpData.ofUtf8("redir-body")); + } + throw new IllegalStateException("cannot regenerate body for redirect"); + }; + + final WebClient client = + WebClient.builder(server.httpUri()) + .followRedirects() + .build(); + + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + streamingOptions()) + .aggregate().join()) + .getRootCause() + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("cannot regenerate body for redirect"); + } + @Test void directConsumeWithoutDecoratorSendsBodyOnce() { // No retry/redirect decorator, so the request is consumed directly via its lazyBody delegate diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java index 4bd2af1e86a..f3c982f372b 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java @@ -97,7 +97,9 @@ void retriesAfterFirstAttemptBodyFailsMidStream() { assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("hello-body"); - // Body regenerated at least twice: the faulted first attempt and the successful retry. - assertThat(bodyCalls).hasValueGreaterThanOrEqualTo(2); + // Body regenerated exactly twice: the faulted first attempt and the successful retry. Assert + // an exact count (not >= 2) so an over-regeneration bug that re-invokes the factory a third + // time — leaking a fresh body resource per attempt — is caught rather than masked. + assertThat(bodyCalls).hasValue(2); } } diff --git a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java index af6f31c6205..2233340d873 100644 --- a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java @@ -19,6 +19,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletionException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; @@ -96,6 +99,32 @@ void duplicateAfterCloseThrows() { assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class); } + @Test + void duplicateAfterAbortTearsDownProducedBodyWithCause() { + // The instance lock guards a duplicate() that races an abort(cause): the just-produced body + // must be torn down with the remembered cause (so an open-file body is released) and duplicate() + // must throw. Covers the abortCause-propagation branch that the close() variant above does not. + final List> produced = new ArrayList<>(); + final Supplier> factory = () -> { + final StreamMessage body = StreamMessage.of(HttpData.ofUtf8("body")); + produced.add(body); + return body; + }; + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + + final RuntimeException cause = new RuntimeException("cleanup"); + dup.abort(cause); + // duplicate() still runs the factory (outside the lock), then observes the aborted state, tears + // the just-produced body down with the cause, and throws. + assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class); + assertThat(produced).hasSize(1); + assertThat(produced.get(0).whenComplete()).isCompletedExceptionally(); + assertThatThrownBy(() -> produced.get(0).whenComplete().join()) + .isInstanceOf(CompletionException.class) + .hasRootCause(cause); + } + @Test void abortReleasesAllOutstandingUnsubscribedRequests() { final Supplier> factory = From 9592a79607bbb544a4ded5d4b3a569d122b86e99 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 4 Aug 2026 06:36:11 +0000 Subject: [PATCH 16/19] test: add deterministic concurrent abort/duplicate race test; drop redundant test Second-round review follow-up (test-only): - concurrentAbortDuringDuplicateTearsDownProducedBody: a barrier-gated test that actually runs duplicate() on one thread while abort(cause) runs on another, forcing the interleave the instance lock exists for. Verified non-vacuous by mutation: with the synchronized guards removed it fails (the produced body leaks and duplicate() does not throw). The prior sequential test only covered the non-interleaved branch. - Remove streamsBodyWithoutAccumulatingIt: a 64 KiB aggregate is delivered identically by a buffering duplicator, so it could not detect a regression to buffering. The companion toDuplicatorIgnoresMaxRequestLength (cap=8) is the real non-buffering proof, and body integrity is already covered by everyDuplicateProducesAFreshBody. Co-authored-by: Isaac --- ...ReproducibleHttpRequestDuplicatorTest.java | 80 ++++++++++++++----- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java index 2233340d873..bb2f2cee31c 100644 --- a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java @@ -22,6 +22,13 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletionException; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; @@ -125,6 +132,61 @@ void duplicateAfterAbortTearsDownProducedBodyWithCause() { .hasRootCause(cause); } + @Test + void concurrentAbortDuringDuplicateTearsDownProducedBody() throws Exception { + // Deterministically reproduce the interleave the instance lock exists for: a caller thread is + // inside duplicate() (the factory has produced a body but the child is not yet registered) while + // the event-loop thread calls abort(cause). The factory blocks on a barrier until abort() has + // fully completed, so duplicate() is guaranteed to observe the aborted state under the lock, + // tear the just-produced body down with the cause, and throw. A regression that dropped + // synchronized or registered the child outside the lock would leak the produced body here. + final CyclicBarrier factoryEntered = new CyclicBarrier(2); + final CountDownLatch abortDone = new CountDownLatch(1); + final List> produced = new CopyOnWriteArrayList<>(); + final Supplier> factory = () -> { + final StreamMessage body = StreamMessage.of(HttpData.ofUtf8("body")); + produced.add(body); + try { + // Signal that the body has been produced, then wait until abort() has finished before + // duplicate() proceeds to the synchronized closed-state check. + factoryEntered.await(10, TimeUnit.SECONDS); + abortDone.await(10, TimeUnit.SECONDS); + } catch (Exception e) { + throw new RuntimeException(e); + } + return body; + }; + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final RuntimeException cause = new RuntimeException("cleanup"); + + final ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + final Future duplicateResult = executor.submit(() -> { + try { + dup.duplicate(); + return null; + } catch (Throwable t) { + return t; + } + }); + + factoryEntered.await(10, TimeUnit.SECONDS); + dup.abort(cause); + abortDone.countDown(); + + assertThat(duplicateResult.get(10, TimeUnit.SECONDS)) + .isInstanceOf(IllegalStateException.class); + assertThat(produced).hasSize(1); + assertThat(produced.get(0).whenComplete()).isCompletedExceptionally(); + assertThatThrownBy(() -> produced.get(0).whenComplete().join()) + .isInstanceOf(CompletionException.class) + .hasRootCause(cause); + } finally { + executor.shutdownNow(); + } + } + @Test void abortReleasesAllOutstandingUnsubscribedRequests() { final Supplier> factory = @@ -156,24 +218,6 @@ void closeLeavesOutstandingRequestsActive() { assertThat(produced.whenComplete()).isNotDone(); } - @Test - void streamsBodyWithoutAccumulatingIt() { - // The whole point of the reproducible duplicator is to avoid buffering the body for replay (and - // thus the ~2 GiB int32 cap of DefaultStreamMessageDuplicator). A produced request must stream - // its body straight through rather than accumulate it; here we simply assert a large body is - // delivered intact. See ReproducibleHttpRequestClientTest#toDuplicatorIgnoresMaxRequestLength - // for the companion proof that the maxRequestLength cap is not applied. - final byte[] large = new byte[64 * 1024]; - final Supplier> factory = - () -> StreamMessage.of(HttpData.wrap(large)); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); - - final HttpRequest produced = dup.duplicate(); - final int received = produced.aggregate().join().content().length(); - assertThat(received).isEqualTo(large.length); - } - @Test void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { final Supplier> factory = From 8a66f3e0e249badcab3f12f75a3755e9a1780ece Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Mon, 10 Aug 2026 18:47:28 +0000 Subject: [PATCH 17/19] refactor: rename reproducible -> defer per review; scope content-length caveat Address jrhee17's review round: - Rename the public @UnstableApi factory HttpRequest.reproducible(...) to HttpRequest.defer(...), aligning with Reactor's Flux#defer and a future StreamMessage#defer. Rename the internal ReproducibleHttpRequest -> DeferredHttpRequest and ReproducibleHttpRequestDuplicator -> DeferredHttpRequestDuplicator (plus the three test classes) so the vocabulary is consistent end to end. "Regenerate" remains the verb for producing an equivalent body per attempt, where "defer" (laziness) would not fit. - Scope the content-length caveat: a streaming request without an explicit content-length is sent chunked (self-delimiting), so a body-length difference is harmless to framing. The framing-corruption warning now applies only to the case where the caller sets content-length explicitly. No behavioral change; the eager per-attempt duplicator design is unchanged. Co-authored-by: Isaac --- .../armeria/client/RedirectingClient.java | 4 +- .../armeria/client/retry/RetryingClient.java | 4 +- ...pRequest.java => DeferredHttpRequest.java} | 22 ++++---- ...ava => DeferredHttpRequestDuplicator.java} | 6 +-- .../linecorp/armeria/common/HttpRequest.java | 32 +++++------ ...ava => DeferredHttpRequestClientTest.java} | 54 +++++++++---------- ...java => DeferredHttpRequestRetryTest.java} | 4 +- ...=> DeferredHttpRequestDuplicatorTest.java} | 46 ++++++++-------- 8 files changed, 87 insertions(+), 85 deletions(-) rename core/src/main/java/com/linecorp/armeria/common/{ReproducibleHttpRequest.java => DeferredHttpRequest.java} (87%) rename core/src/main/java/com/linecorp/armeria/common/{ReproducibleHttpRequestDuplicator.java => DeferredHttpRequestDuplicator.java} (96%) rename core/src/test/java/com/linecorp/armeria/client/{ReproducibleHttpRequestClientTest.java => DeferredHttpRequestClientTest.java} (91%) rename core/src/test/java/com/linecorp/armeria/client/retry/{ReproducibleHttpRequestRetryTest.java => DeferredHttpRequestRetryTest.java} (97%) rename core/src/test/java/com/linecorp/armeria/common/{ReproducibleHttpRequestDuplicatorTest.java => DeferredHttpRequestDuplicatorTest.java} (87%) diff --git a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java index b17ce63f82d..572a7ebf5b9 100644 --- a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java @@ -188,8 +188,8 @@ private void execute0(ClientRequestContext ctx, RedirectContext redirectCtx, final HttpRequest duplicateReq; final ClientRequestContext derivedCtx; try { - // duplicate() may throw if the request body cannot be reproduced (see - // HttpRequest.reproducible); fail the request instead of following the redirect. + // duplicate() may throw if the request body cannot be regenerated (see + // HttpRequest#defer); fail the request instead of following the redirect. duplicateReq = reqDuplicator.duplicate(); derivedCtx = ClientUtil.newDerivedContext(ctx, duplicateReq, ctx.rpcRequest(), initialAttempt); } catch (Throwable t) { diff --git a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java index cbea9d305f0..378997fb8a8 100644 --- a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java @@ -298,8 +298,8 @@ private void doExecute0(ClientRequestContext ctx, HttpRequestDuplicator rootReqD final HttpRequest duplicateReq; final ClientRequestContext derivedCtx; try { - // duplicate() may throw if the request body cannot be reproduced (see - // HttpRequest.reproducible); fail the request instead of retrying an unreproducible body. + // duplicate() may throw if the request body cannot be regenerated (see + // HttpRequest#defer); fail the request instead of retrying a non-regenerable body. if (initialAttempt) { duplicateReq = rootReqDuplicator.duplicate(); } else { diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java similarity index 87% rename from core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java rename to core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java index c09a06cefc2..e4e09e0e4aa 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java @@ -29,14 +29,14 @@ import io.netty.util.concurrent.EventExecutor; /** - * An {@link HttpRequest} whose body can be reproduced on demand, so {@code RetryingClient} and + * An {@link HttpRequest} whose body is regenerated on demand, so {@code RetryingClient} and * {@code RedirectingClient} can resend it without buffering the whole body in memory. * - *

See {@link HttpRequest#reproducible(RequestHeaders, Supplier)} for details and usage. + *

See {@link HttpRequest#defer(RequestHeaders, Supplier)} for details and usage. * *

The body factory is invoked lazily: it is never called during construction. On the * retry/redirect path {@link #toDuplicator(EventExecutor, long)} returns a - * {@link ReproducibleHttpRequestDuplicator} that calls the factory once per attempt, and this + * {@link DeferredHttpRequestDuplicator} that calls the factory once per attempt, and this * request's own delegate is never subscribed. When this request is instead consumed directly (no * retry/redirect decorator), the factory is invoked once when the delegate is first subscribed to * produce the single body. @@ -48,13 +48,13 @@ * regenerate on a subsequent subscribe, because a stream permits only one subscription). The factory * therefore runs at most once on the direct path. */ -final class ReproducibleHttpRequest +final class DeferredHttpRequest extends NonOverridableStreamMessageWrapper implements HttpRequest { private final RequestHeaders headers; private final Supplier> bodyFactory; - ReproducibleHttpRequest(RequestHeaders headers, + DeferredHttpRequest(RequestHeaders headers, Supplier> bodyFactory) { super(lazyBody(bodyFactory)); this.headers = headers; @@ -84,9 +84,9 @@ private static StreamMessage lazyBody( final StreamMessage cast = (StreamMessage) body; // This single-arg subscribe forwards neither the caller's SubscriptionOptions // (WITH_POOLED_OBJECTS / NOTIFY_CANCELLATION) nor the requested executor to the produced - // body. That is acceptable only because this is a cold path: a reproducible request is + // body. That is acceptable only because this is a cold path: a deferred request is // meant to be driven by a retry/redirect decorator, which uses toDuplicator(...) and never - // subscribes this delegate. This branch is reached only when a reproducible request is + // subscribes this delegate. This branch is reached only when a deferred request is // consumed directly, with no such decorator. cast.subscribe(subscriber); }); @@ -103,13 +103,13 @@ public HttpRequest withHeaders(RequestHeaders newHeaders) { if (headers == newHeaders) { return this; } - // Preserve reproducibility across a header rewrite (e.g. a base-URI path prefix applied by + // Preserve the deferred body across a header rewrite (e.g. a base-URI path prefix applied by // DefaultWebClient, or a redirect/retry decorator overriding the path). The default // HttpRequest.withHeaders wraps this in a HeaderOverridingHttpRequest, which does not override // toDuplicator and would therefore fall back to the buffering DefaultHttpRequestDuplicator, // reintroducing the ~2 GiB size limit this request type exists to avoid. Rebinding the same // factory to the new headers keeps the non-buffering toDuplicator path. - return new ReproducibleHttpRequest(newHeaders, bodyFactory); + return new DeferredHttpRequest(newHeaders, bodyFactory); } @SuppressWarnings("unchecked") @@ -125,9 +125,9 @@ public HttpRequestDuplicator toDuplicator(EventExecutor executor) { @Override public HttpRequestDuplicator toDuplicator(EventExecutor executor, long maxRequestLength) { - // Neither argument applies: the reproducible duplicator never buffers, so it needs no + // Neither argument applies: the deferred duplicator never buffers, so it needs no // subscriber executor and has no accumulated length to cap. Each attempt streams a fresh // body straight from the factory. - return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); + return new DeferredHttpRequestDuplicator(headers, bodyFactory); } } diff --git a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java similarity index 96% rename from core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java rename to core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java index 896317b161d..f02a7ab8188 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java +++ b/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java @@ -29,7 +29,7 @@ import com.linecorp.armeria.common.stream.StreamMessage; /** - * An {@link HttpRequestDuplicator} that reproduces the request body without buffering it. Every + * An {@link HttpRequestDuplicator} that regenerates the request body without buffering it. Every * {@link #duplicate()} — including the first — obtains a fresh body from the supplied factory, so no * attempt reuses another attempt's stream and the original request handed to the client is never put * on the wire. This avoids the ~2 GiB {@code int} size limit and the memory cost of @@ -49,7 +49,7 @@ * {@code duplicate}, and {@code duplicate} then throws instead of returning a request that would never * be torn down. */ -final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { +final class DeferredHttpRequestDuplicator implements HttpRequestDuplicator { private final RequestHeaders headers; private final Supplier> bodyFactory; @@ -61,7 +61,7 @@ final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { @Nullable private Throwable abortCause; - ReproducibleHttpRequestDuplicator( + DeferredHttpRequestDuplicator( RequestHeaders headers, Supplier> bodyFactory) { this.headers = requireNonNull(headers, "headers"); diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index affda30f859..319b831577b 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -359,7 +359,7 @@ static HttpRequest of(RequestHeaders headers, } /** - * Creates a new {@link HttpRequest} whose body can be reproduced on demand, so that + * Creates a new {@link HttpRequest} whose body is regenerated on demand, so that * {@code RetryingClient} and {@code RedirectingClient} can resend it without buffering the whole * body in memory. * @@ -372,7 +372,7 @@ static HttpRequest of(RequestHeaders headers, * final RequestHeaders headers = * RequestHeaders.of(HttpMethod.POST, "/upload", * HttpHeaderNames.CONTENT_TYPE, "application/octet-stream"); - * final HttpRequest req = HttpRequest.reproducible(headers, () -> StreamMessage.of(path)); + * final HttpRequest req = HttpRequest.defer(headers, () -> StreamMessage.of(path)); * final RequestOptions options = * RequestOptions.builder() * .exchangeType(ExchangeType.REQUEST_STREAMING) @@ -383,30 +383,32 @@ static HttpRequest of(RequestHeaders headers, *

The {@code bodyFactory} is invoked once per attempt (initial request, retry attempt, or * redirect hop). The fixed {@code headers} are reused for every attempt; the factory regenerates * only the body {@link StreamMessage}, so the request method and headers cannot drift between - * attempts. Every invocation must produce an equivalent body — the same bytes and - * trailers, not merely the same length — because the fixed {@code headers} (including any declared - * {@link HttpHeaderNames#CONTENT_LENGTH}) are reused verbatim on every attempt and are not + * attempts. Every invocation should produce an equivalent body — the same bytes and + * trailers — because the fixed {@code headers} are reused verbatim on every attempt and are not * re-validated against the regenerated body. A factory whose output varies across invocations * (e.g. it embeds a timestamp, or reads a file being mutated concurrently) silently sends - * different data on a retry or redirect, and a length mismatch against a declared - * {@code content-length} corrupts wire framing (the request stalls or the next message is - * garbled) with no error surfaced. - * - *

Reproducible replay applies only at the outermost duplicating decorator; each attempt + * different data on a retry or redirect. This is normally harmless to framing, because a streaming + * request without an explicit {@link HttpHeaderNames#CONTENT_LENGTH} is sent with chunked + * transfer-encoding, which is self-delimiting. The one exception is when the caller sets + * {@code content-length} explicitly in {@code headers}: that length is reused verbatim and not + * re-validated, so a body whose length differs from the declared value corrupts wire framing (the + * request stalls or the next message is garbled) with no error surfaced. + * + *

Deferred replay applies only at the outermost duplicating decorator; each attempt * it hands downstream is an ordinary {@link HttpRequest}, so any inner duplicating decorator treats * it as a normal request and buffers it. Which decorator is outermost depends on configuration: *

    *
  • With retries only (the common case; redirects are disabled by default), - * {@code RetryingClient} is the outermost decorator and replays reproducibly across every + * {@code RetryingClient} is the outermost decorator and replays the deferred body across every * retry.
  • *
  • With {@code followRedirects()} enabled, the built-in {@code RedirectingClient} wraps the - * user-supplied decorators (including {@code RetryingClient}), so it replays reproducibly + * user-supplied decorators (including {@code RetryingClient}), so it replays the deferred body * across redirect hops, but an inner {@code RetryingClient} re-buffers the body when retrying * within a single hop — reintroducing the ~2 GiB limit for that retry. If you need * non-buffering retries of a very large body, avoid stacking {@code RetryingClient} beneath * {@code RedirectingClient}.
  • *
- * Reproducibility is honored for streaming requests + * Deferral is honored for streaming requests * ({@link ExchangeType#isRequestStreaming()}); an aggregated exchange type buffers the body as * usual. * @@ -418,12 +420,12 @@ static HttpRequest of(RequestHeaders headers, * without exhausting the remaining retry budget. */ @UnstableApi - static HttpRequest reproducible( + static HttpRequest defer( RequestHeaders headers, Supplier> bodyFactory) { requireNonNull(headers, "headers"); requireNonNull(bodyFactory, "bodyFactory"); - return new ReproducibleHttpRequest(headers, bodyFactory); + return new DeferredHttpRequest(headers, bodyFactory); } /** diff --git a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java similarity index 91% rename from core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java rename to core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java index e1235adb996..07b9285b6f1 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java @@ -51,7 +51,7 @@ import io.netty.util.concurrent.EventExecutor; -class ReproducibleHttpRequestClientTest { +class DeferredHttpRequestClientTest { private static final AtomicInteger serverHits = new AtomicInteger(); @@ -79,7 +79,7 @@ protected void configure(ServerBuilder sb) { HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, agg.contentUtf8()).toHttpResponse()))); // Echoes the concatenated body plus the received trailer, failing the first attempt so the - // multi-chunk body is reproduced across a retry. + // multi-chunk body is regenerated across a retry. sb.service("/multi", (ctx, req) -> HttpResponse.of( req.aggregate().thenApply(agg -> { final int hit = serverHits.incrementAndGet(); @@ -106,7 +106,7 @@ protected void configure(ServerBuilder sb) { HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, agg.contentUtf8()).toHttpResponse()))); // Same fail-once-then-echo behavior as /upload, reached only when a base-URI path prefix - // ("/api") rewrites the request path — the scenario that must keep the reproducible body. + // ("/api") rewrites the request path — the scenario that must keep the deferred body. sb.service("/api/upload", (ctx, req) -> HttpResponse.of( req.aggregate().thenApply(agg -> { final int hit = serverHits.incrementAndGet(); @@ -141,7 +141,7 @@ void factoryNotInvokedEagerly() { }; // Creating the request must not call the factory; it is invoked lazily per attempt only. - final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + final HttpRequest req = HttpRequest.defer(headers, bodyFactory); assertThat(bodyCalls).hasValue(0); req.abort(); } @@ -157,7 +157,7 @@ void factoryInvokedOnceOnDirectConsumption() { // Consuming the request directly (no retry/redirect decorator) subscribes the lazy delegate, // which invokes the factory exactly once to produce the single body. - final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + final HttpRequest req = HttpRequest.defer(headers, bodyFactory); assertThat(req.aggregate().join().contentUtf8()).isEqualTo("hello-body"); assertThat(bodyCalls).hasValue(1); } @@ -174,7 +174,7 @@ void abortBeforeSubscriptionInvokesFactoryOnce() { // Aborting a directly-consumed request that was never sent subscribes an aborting subscriber, // which runs the factory once so the produced body can be released. It does not regenerate on a // later subscribe (a stream permits only one subscription), so the factory runs at most once. - final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); + final HttpRequest req = HttpRequest.defer(headers, bodyFactory); req.abort(); // abort() propagates asynchronously; join() blocks until completion before we assert the count. assertThatThrownBy(() -> req.whenComplete().join()).isInstanceOf(CompletionException.class); @@ -199,7 +199,7 @@ void retryRegeneratesBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -213,13 +213,13 @@ void retryRegeneratesBody() { } @Test - void retryReproducesMultiChunkBodyAndTrailers() { + void retryRegeneratesMultiChunkBodyAndTrailers() { final AtomicInteger bodyCalls = new AtomicInteger(); final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/multi", HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); // A genuinely multi-chunk body terminated by a trailer, the shape a streaming upload takes. - // Each attempt must reproduce every interior chunk in order plus the trailer. + // Each attempt must regenerate every interior chunk in order plus the trailer. final Supplier> bodyFactory = () -> { bodyCalls.incrementAndGet(); return StreamMessage.of(HttpData.ofUtf8("a"), @@ -235,11 +235,11 @@ void retryReproducesMultiChunkBodyAndTrailers() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); - // Concatenated interior chunks (in order) plus the reproduced trailer, on the re-sent attempt. + // Concatenated interior chunks (in order) plus the regenerated trailer, on the re-sent attempt. assertThat(res.contentUtf8()).isEqualTo("abc|v"); // Deterministic: initial + one retry. Exact assertion guards against over-regeneration. assertThat(serverHits).hasValue(2); @@ -268,13 +268,13 @@ void factoryThrowingOnRetryFailsFast() { // The surfaced failure must carry the factory's own exception, not merely be "some Exception": // a connection error, timeout, or unrelated bug would also satisfy isInstanceOf(Exception.class). - assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() .isInstanceOf(IllegalStateException.class) .hasMessageContaining("cannot regenerate body"); - // The factory is consulted twice: once for the initial body (via reproducible()), once for the + // The factory is consulted twice: once for the initial body (via defer()), once for the // failing retry. It fails fast rather than looping through the whole retry budget. assertThat(bodyCalls).hasValue(2); } @@ -296,7 +296,7 @@ void followsRedirectRegeneratingBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -323,7 +323,7 @@ void seeOtherRedirectDropsBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); // On a 303 the method is rewritten to GET and the body dropped; the duplicator is aborted @@ -346,7 +346,7 @@ void stackedRetryAndRedirect() { return StreamMessage.of(HttpData.ofUtf8("redir-body")); }; - // Both RetryingClient and RedirectingClient present; the reproducible body must be resent + // Both RetryingClient and RedirectingClient present; the deferred body must be resent // correctly through the redirect without a destructive double-consume. final WebClient client = WebClient.builder(server.httpUri()) @@ -356,7 +356,7 @@ void stackedRetryAndRedirect() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -388,7 +388,7 @@ void factoryThrowingOnRedirectHopFailsFast() { .followRedirects() .build(); - assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -412,7 +412,7 @@ void directConsumeWithoutDecoratorSendsBodyOnce() { final WebClient client = WebClient.of(server.httpUri()); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -432,7 +432,7 @@ void directConsumeSurfacesThrowingFactory() { }; final WebClient client = WebClient.of(server.httpUri()); - assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -450,7 +450,7 @@ void directConsumeSurfacesNullFactory() { final Supplier> bodyFactory = () -> null; final WebClient client = WebClient.of(server.httpUri()); - assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -459,7 +459,7 @@ void directConsumeSurfacesNullFactory() { @Test void toDuplicatorIgnoresMaxRequestLength() { - // The reproducible duplicator never buffers, so it must ignore the maxRequestLength cap that a + // The deferred duplicator never buffers, so it must ignore the maxRequestLength cap that a // buffering DefaultStreamMessageDuplicator would enforce. A body far larger than a tiny cap must // still stream to completion; a regression that fell back to a buffering duplicator would throw // ContentTooLargeException here. @@ -470,7 +470,7 @@ void toDuplicatorIgnoresMaxRequestLength() { final EventExecutor executor = CommonPools.workerGroup().next(); final HttpRequestDuplicator duplicator = - HttpRequest.reproducible(headers, bodyFactory).toDuplicator(executor, 8); + HttpRequest.defer(headers, bodyFactory).toDuplicator(executor, 8); final AggregatedHttpRequest produced = duplicator.duplicate().aggregate().join(); assertThat(produced.content().length()).isEqualTo(large.length); @@ -478,9 +478,9 @@ void toDuplicatorIgnoresMaxRequestLength() { } @Test - void basePathPrefixRemainsReproducible() { + void basePathPrefixRemainsDeferred() { // A WebClient built with a base-URI path prefix rewrites the request path via - // req.withHeaders(...). If ReproducibleHttpRequest did not override withHeaders, the rewritten + // req.withHeaders(...). If DeferredHttpRequest did not override withHeaders, the rewritten // request would be a plain HeaderOverridingHttpRequest whose toDuplicator falls back to the // buffering DefaultStreamMessageDuplicator — silently reintroducing the ~2 GiB limit. This test // pins that the rewritten request still regenerates its body per attempt (non-buffering path). @@ -502,13 +502,13 @@ void basePathPrefixRemainsReproducible() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("prefixed-body"); // Regenerated for the initial attempt and the retry — proving the path-rewritten request kept - // the reproducible (non-buffering) duplicator rather than falling back to buffering. + // the deferred (non-buffering) duplicator rather than falling back to buffering. assertThat(bodyCalls).hasValue(2); } } diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java similarity index 97% rename from core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java rename to core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java index f3c982f372b..f5b9e7d067a 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java @@ -41,7 +41,7 @@ import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.testing.junit5.server.ServerExtension; -class ReproducibleHttpRequestRetryTest { +class DeferredHttpRequestRetryTest { @RegisterExtension static final ServerExtension server = new ServerExtension() { @@ -92,7 +92,7 @@ void retriesAfterFirstAttemptBodyFailsMidStream() { .exchangeType(ExchangeType.REQUEST_STREAMING) .build(); - final HttpRequest req = HttpRequest.reproducible(headers, bodySupplier); + final HttpRequest req = HttpRequest.defer(headers, bodySupplier); final AggregatedHttpResponse res = client.execute(req, options).aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); diff --git a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java similarity index 87% rename from core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java rename to core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java index bb2f2cee31c..cd40ea09114 100644 --- a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java @@ -36,7 +36,7 @@ import com.linecorp.armeria.common.stream.StreamMessage; -class ReproducibleHttpRequestDuplicatorTest { +class DeferredHttpRequestDuplicatorTest { private static final RequestHeaders HEADERS = RequestHeaders.of(HttpMethod.POST, "/upload"); @@ -47,8 +47,8 @@ void everyDuplicateProducesAFreshBody() { calls.incrementAndGet(); return StreamMessage.of(HttpData.ofUtf8("body")); }; - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); // Even the first duplicate() invokes the factory — the caller's request is never reused. final HttpRequest first = dup.duplicate(); @@ -63,8 +63,8 @@ void everyDuplicateProducesAFreshBody() { void duplicateWithHeadersOverridesHeaders() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); final RequestHeaders overridden = RequestHeaders.of(HttpMethod.POST, "/upload", "x-attempt", "1"); final HttpRequest req = dup.duplicate(overridden); @@ -74,21 +74,21 @@ void duplicateWithHeadersOverridesHeaders() { @Test void factoryThrowingPropagates() { final Supplier> factory = () -> { - throw new IllegalStateException("cannot reproduce body"); + throw new IllegalStateException("cannot regenerate body"); }; - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); // Fail fast: the exception propagates so the client can terminate the request. assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class) - .hasMessageContaining("cannot reproduce body"); + .hasMessageContaining("cannot regenerate body"); } @Test void factoryReturningNullThrows() { final Supplier> factory = () -> null; - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); assertThatThrownBy(dup::duplicate).isInstanceOf(NullPointerException.class); } @@ -97,8 +97,8 @@ void factoryReturningNullThrows() { void duplicateAfterCloseThrows() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); dup.duplicate(); dup.close(); @@ -117,8 +117,8 @@ void duplicateAfterAbortTearsDownProducedBodyWithCause() { produced.add(body); return body; }; - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); final RuntimeException cause = new RuntimeException("cleanup"); dup.abort(cause); @@ -156,8 +156,8 @@ void concurrentAbortDuringDuplicateTearsDownProducedBody() throws Exception { } return body; }; - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); final RuntimeException cause = new RuntimeException("cleanup"); final ExecutorService executor = Executors.newSingleThreadExecutor(); @@ -191,8 +191,8 @@ void concurrentAbortDuringDuplicateTearsDownProducedBody() throws Exception { void abortReleasesAllOutstandingUnsubscribedRequests() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); // Multiple produced requests may be outstanding at once (e.g. hedging); abort() must tear // down every one of them, not just the most recently produced, so no body is leaked. @@ -208,8 +208,8 @@ void abortReleasesAllOutstandingUnsubscribedRequests() { void closeLeavesOutstandingRequestsActive() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); final HttpRequest produced = dup.duplicate(); // StreamMessageDuplicator contract: close() prevents further duplication but must not abort @@ -222,8 +222,8 @@ void closeLeavesOutstandingRequestsActive() { void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final ReproducibleHttpRequestDuplicator dup = - new ReproducibleHttpRequestDuplicator(HEADERS, factory); + final DeferredHttpRequestDuplicator dup = + new DeferredHttpRequestDuplicator(HEADERS, factory); // Draining a produced request completes it; it is then removed from the tracked set so the set // does not grow unbounded and a later abort() leaves the already-completed request untouched. From 0cd4d64e81a15699ea9abcc92be2db4f6f97dfa7 Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 11 Aug 2026 18:23:24 +0000 Subject: [PATCH 18/19] revert: restore `reproducible` naming per maintainer request; keep content-length fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit jrhee17 and minwoox asked to revert the `defer` rename: StreamMessage is single-subscription by contract, so the Flux#defer mental model (re-subscribe -> regenerate) does not apply, and `defer` says nothing about the equivalent-body contract that actually defines this type. `reproducible` names that contract directly. This fully reverts commit 8a66f3e0e's rename — HttpRequest.defer(...) is back to HttpRequest.reproducible(...), and the DeferredHttpRequest* classes/tests are back to ReproducibleHttpRequest*. The content-length caveat scoping from that commit is retained (the framing warning applies only when the caller sets content-length explicitly; otherwise the request is chunked and self-delimiting). Co-authored-by: Isaac --- .../armeria/client/RedirectingClient.java | 4 +- .../armeria/client/retry/RetryingClient.java | 4 +- .../linecorp/armeria/common/HttpRequest.java | 16 +++--- ...uest.java => ReproducibleHttpRequest.java} | 22 ++++---- ...=> ReproducibleHttpRequestDuplicator.java} | 6 +-- ...=> ReproducibleHttpRequestClientTest.java} | 54 +++++++++---------- ... => ReproducibleHttpRequestRetryTest.java} | 4 +- ...eproducibleHttpRequestDuplicatorTest.java} | 46 ++++++++-------- 8 files changed, 78 insertions(+), 78 deletions(-) rename core/src/main/java/com/linecorp/armeria/common/{DeferredHttpRequest.java => ReproducibleHttpRequest.java} (87%) rename core/src/main/java/com/linecorp/armeria/common/{DeferredHttpRequestDuplicator.java => ReproducibleHttpRequestDuplicator.java} (96%) rename core/src/test/java/com/linecorp/armeria/client/{DeferredHttpRequestClientTest.java => ReproducibleHttpRequestClientTest.java} (91%) rename core/src/test/java/com/linecorp/armeria/client/retry/{DeferredHttpRequestRetryTest.java => ReproducibleHttpRequestRetryTest.java} (97%) rename core/src/test/java/com/linecorp/armeria/common/{DeferredHttpRequestDuplicatorTest.java => ReproducibleHttpRequestDuplicatorTest.java} (87%) diff --git a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java index 572a7ebf5b9..b17ce63f82d 100644 --- a/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/RedirectingClient.java @@ -188,8 +188,8 @@ private void execute0(ClientRequestContext ctx, RedirectContext redirectCtx, final HttpRequest duplicateReq; final ClientRequestContext derivedCtx; try { - // duplicate() may throw if the request body cannot be regenerated (see - // HttpRequest#defer); fail the request instead of following the redirect. + // duplicate() may throw if the request body cannot be reproduced (see + // HttpRequest.reproducible); fail the request instead of following the redirect. duplicateReq = reqDuplicator.duplicate(); derivedCtx = ClientUtil.newDerivedContext(ctx, duplicateReq, ctx.rpcRequest(), initialAttempt); } catch (Throwable t) { diff --git a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java index 378997fb8a8..cbea9d305f0 100644 --- a/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java @@ -298,8 +298,8 @@ private void doExecute0(ClientRequestContext ctx, HttpRequestDuplicator rootReqD final HttpRequest duplicateReq; final ClientRequestContext derivedCtx; try { - // duplicate() may throw if the request body cannot be regenerated (see - // HttpRequest#defer); fail the request instead of retrying a non-regenerable body. + // duplicate() may throw if the request body cannot be reproduced (see + // HttpRequest.reproducible); fail the request instead of retrying an unreproducible body. if (initialAttempt) { duplicateReq = rootReqDuplicator.duplicate(); } else { diff --git a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java index 319b831577b..5864f4519b8 100644 --- a/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/HttpRequest.java @@ -359,7 +359,7 @@ static HttpRequest of(RequestHeaders headers, } /** - * Creates a new {@link HttpRequest} whose body is regenerated on demand, so that + * Creates a new {@link HttpRequest} whose body can be reproduced on demand, so that * {@code RetryingClient} and {@code RedirectingClient} can resend it without buffering the whole * body in memory. * @@ -372,7 +372,7 @@ static HttpRequest of(RequestHeaders headers, * final RequestHeaders headers = * RequestHeaders.of(HttpMethod.POST, "/upload", * HttpHeaderNames.CONTENT_TYPE, "application/octet-stream"); - * final HttpRequest req = HttpRequest.defer(headers, () -> StreamMessage.of(path)); + * final HttpRequest req = HttpRequest.reproducible(headers, () -> StreamMessage.of(path)); * final RequestOptions options = * RequestOptions.builder() * .exchangeType(ExchangeType.REQUEST_STREAMING) @@ -394,21 +394,21 @@ static HttpRequest of(RequestHeaders headers, * re-validated, so a body whose length differs from the declared value corrupts wire framing (the * request stalls or the next message is garbled) with no error surfaced. * - *

Deferred replay applies only at the outermost duplicating decorator; each attempt + *

Reproducible replay applies only at the outermost duplicating decorator; each attempt * it hands downstream is an ordinary {@link HttpRequest}, so any inner duplicating decorator treats * it as a normal request and buffers it. Which decorator is outermost depends on configuration: *

    *
  • With retries only (the common case; redirects are disabled by default), - * {@code RetryingClient} is the outermost decorator and replays the deferred body across every + * {@code RetryingClient} is the outermost decorator and replays reproducibly across every * retry.
  • *
  • With {@code followRedirects()} enabled, the built-in {@code RedirectingClient} wraps the - * user-supplied decorators (including {@code RetryingClient}), so it replays the deferred body + * user-supplied decorators (including {@code RetryingClient}), so it replays reproducibly * across redirect hops, but an inner {@code RetryingClient} re-buffers the body when retrying * within a single hop — reintroducing the ~2 GiB limit for that retry. If you need * non-buffering retries of a very large body, avoid stacking {@code RetryingClient} beneath * {@code RedirectingClient}.
  • *
- * Deferral is honored for streaming requests + * Reproducibility is honored for streaming requests * ({@link ExchangeType#isRequestStreaming()}); an aggregated exchange type buffers the body as * usual. * @@ -420,12 +420,12 @@ static HttpRequest of(RequestHeaders headers, * without exhausting the remaining retry budget. */ @UnstableApi - static HttpRequest defer( + static HttpRequest reproducible( RequestHeaders headers, Supplier> bodyFactory) { requireNonNull(headers, "headers"); requireNonNull(bodyFactory, "bodyFactory"); - return new DeferredHttpRequest(headers, bodyFactory); + return new ReproducibleHttpRequest(headers, bodyFactory); } /** diff --git a/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java similarity index 87% rename from core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java rename to core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java index e4e09e0e4aa..c09a06cefc2 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequest.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequest.java @@ -29,14 +29,14 @@ import io.netty.util.concurrent.EventExecutor; /** - * An {@link HttpRequest} whose body is regenerated on demand, so {@code RetryingClient} and + * An {@link HttpRequest} whose body can be reproduced on demand, so {@code RetryingClient} and * {@code RedirectingClient} can resend it without buffering the whole body in memory. * - *

See {@link HttpRequest#defer(RequestHeaders, Supplier)} for details and usage. + *

See {@link HttpRequest#reproducible(RequestHeaders, Supplier)} for details and usage. * *

The body factory is invoked lazily: it is never called during construction. On the * retry/redirect path {@link #toDuplicator(EventExecutor, long)} returns a - * {@link DeferredHttpRequestDuplicator} that calls the factory once per attempt, and this + * {@link ReproducibleHttpRequestDuplicator} that calls the factory once per attempt, and this * request's own delegate is never subscribed. When this request is instead consumed directly (no * retry/redirect decorator), the factory is invoked once when the delegate is first subscribed to * produce the single body. @@ -48,13 +48,13 @@ * regenerate on a subsequent subscribe, because a stream permits only one subscription). The factory * therefore runs at most once on the direct path. */ -final class DeferredHttpRequest +final class ReproducibleHttpRequest extends NonOverridableStreamMessageWrapper implements HttpRequest { private final RequestHeaders headers; private final Supplier> bodyFactory; - DeferredHttpRequest(RequestHeaders headers, + ReproducibleHttpRequest(RequestHeaders headers, Supplier> bodyFactory) { super(lazyBody(bodyFactory)); this.headers = headers; @@ -84,9 +84,9 @@ private static StreamMessage lazyBody( final StreamMessage cast = (StreamMessage) body; // This single-arg subscribe forwards neither the caller's SubscriptionOptions // (WITH_POOLED_OBJECTS / NOTIFY_CANCELLATION) nor the requested executor to the produced - // body. That is acceptable only because this is a cold path: a deferred request is + // body. That is acceptable only because this is a cold path: a reproducible request is // meant to be driven by a retry/redirect decorator, which uses toDuplicator(...) and never - // subscribes this delegate. This branch is reached only when a deferred request is + // subscribes this delegate. This branch is reached only when a reproducible request is // consumed directly, with no such decorator. cast.subscribe(subscriber); }); @@ -103,13 +103,13 @@ public HttpRequest withHeaders(RequestHeaders newHeaders) { if (headers == newHeaders) { return this; } - // Preserve the deferred body across a header rewrite (e.g. a base-URI path prefix applied by + // Preserve reproducibility across a header rewrite (e.g. a base-URI path prefix applied by // DefaultWebClient, or a redirect/retry decorator overriding the path). The default // HttpRequest.withHeaders wraps this in a HeaderOverridingHttpRequest, which does not override // toDuplicator and would therefore fall back to the buffering DefaultHttpRequestDuplicator, // reintroducing the ~2 GiB size limit this request type exists to avoid. Rebinding the same // factory to the new headers keeps the non-buffering toDuplicator path. - return new DeferredHttpRequest(newHeaders, bodyFactory); + return new ReproducibleHttpRequest(newHeaders, bodyFactory); } @SuppressWarnings("unchecked") @@ -125,9 +125,9 @@ public HttpRequestDuplicator toDuplicator(EventExecutor executor) { @Override public HttpRequestDuplicator toDuplicator(EventExecutor executor, long maxRequestLength) { - // Neither argument applies: the deferred duplicator never buffers, so it needs no + // Neither argument applies: the reproducible duplicator never buffers, so it needs no // subscriber executor and has no accumulated length to cap. Each attempt streams a fresh // body straight from the factory. - return new DeferredHttpRequestDuplicator(headers, bodyFactory); + return new ReproducibleHttpRequestDuplicator(headers, bodyFactory); } } diff --git a/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java similarity index 96% rename from core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java rename to core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java index f02a7ab8188..896317b161d 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicator.java +++ b/core/src/main/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicator.java @@ -29,7 +29,7 @@ import com.linecorp.armeria.common.stream.StreamMessage; /** - * An {@link HttpRequestDuplicator} that regenerates the request body without buffering it. Every + * An {@link HttpRequestDuplicator} that reproduces the request body without buffering it. Every * {@link #duplicate()} — including the first — obtains a fresh body from the supplied factory, so no * attempt reuses another attempt's stream and the original request handed to the client is never put * on the wire. This avoids the ~2 GiB {@code int} size limit and the memory cost of @@ -49,7 +49,7 @@ * {@code duplicate}, and {@code duplicate} then throws instead of returning a request that would never * be torn down. */ -final class DeferredHttpRequestDuplicator implements HttpRequestDuplicator { +final class ReproducibleHttpRequestDuplicator implements HttpRequestDuplicator { private final RequestHeaders headers; private final Supplier> bodyFactory; @@ -61,7 +61,7 @@ final class DeferredHttpRequestDuplicator implements HttpRequestDuplicator { @Nullable private Throwable abortCause; - DeferredHttpRequestDuplicator( + ReproducibleHttpRequestDuplicator( RequestHeaders headers, Supplier> bodyFactory) { this.headers = requireNonNull(headers, "headers"); diff --git a/core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java similarity index 91% rename from core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java rename to core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java index 07b9285b6f1..e1235adb996 100644 --- a/core/src/test/java/com/linecorp/armeria/client/DeferredHttpRequestClientTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ReproducibleHttpRequestClientTest.java @@ -51,7 +51,7 @@ import io.netty.util.concurrent.EventExecutor; -class DeferredHttpRequestClientTest { +class ReproducibleHttpRequestClientTest { private static final AtomicInteger serverHits = new AtomicInteger(); @@ -79,7 +79,7 @@ protected void configure(ServerBuilder sb) { HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, agg.contentUtf8()).toHttpResponse()))); // Echoes the concatenated body plus the received trailer, failing the first attempt so the - // multi-chunk body is regenerated across a retry. + // multi-chunk body is reproduced across a retry. sb.service("/multi", (ctx, req) -> HttpResponse.of( req.aggregate().thenApply(agg -> { final int hit = serverHits.incrementAndGet(); @@ -106,7 +106,7 @@ protected void configure(ServerBuilder sb) { HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, agg.contentUtf8()).toHttpResponse()))); // Same fail-once-then-echo behavior as /upload, reached only when a base-URI path prefix - // ("/api") rewrites the request path — the scenario that must keep the deferred body. + // ("/api") rewrites the request path — the scenario that must keep the reproducible body. sb.service("/api/upload", (ctx, req) -> HttpResponse.of( req.aggregate().thenApply(agg -> { final int hit = serverHits.incrementAndGet(); @@ -141,7 +141,7 @@ void factoryNotInvokedEagerly() { }; // Creating the request must not call the factory; it is invoked lazily per attempt only. - final HttpRequest req = HttpRequest.defer(headers, bodyFactory); + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); assertThat(bodyCalls).hasValue(0); req.abort(); } @@ -157,7 +157,7 @@ void factoryInvokedOnceOnDirectConsumption() { // Consuming the request directly (no retry/redirect decorator) subscribes the lazy delegate, // which invokes the factory exactly once to produce the single body. - final HttpRequest req = HttpRequest.defer(headers, bodyFactory); + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); assertThat(req.aggregate().join().contentUtf8()).isEqualTo("hello-body"); assertThat(bodyCalls).hasValue(1); } @@ -174,7 +174,7 @@ void abortBeforeSubscriptionInvokesFactoryOnce() { // Aborting a directly-consumed request that was never sent subscribes an aborting subscriber, // which runs the factory once so the produced body can be released. It does not regenerate on a // later subscribe (a stream permits only one subscription), so the factory runs at most once. - final HttpRequest req = HttpRequest.defer(headers, bodyFactory); + final HttpRequest req = HttpRequest.reproducible(headers, bodyFactory); req.abort(); // abort() propagates asynchronously; join() blocks until completion before we assert the count. assertThatThrownBy(() -> req.whenComplete().join()).isInstanceOf(CompletionException.class); @@ -199,7 +199,7 @@ void retryRegeneratesBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -213,13 +213,13 @@ void retryRegeneratesBody() { } @Test - void retryRegeneratesMultiChunkBodyAndTrailers() { + void retryReproducesMultiChunkBodyAndTrailers() { final AtomicInteger bodyCalls = new AtomicInteger(); final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, "/multi", HttpHeaderNames.CONTENT_TYPE, MediaType.PLAIN_TEXT_UTF_8); // A genuinely multi-chunk body terminated by a trailer, the shape a streaming upload takes. - // Each attempt must regenerate every interior chunk in order plus the trailer. + // Each attempt must reproduce every interior chunk in order plus the trailer. final Supplier> bodyFactory = () -> { bodyCalls.incrementAndGet(); return StreamMessage.of(HttpData.ofUtf8("a"), @@ -235,11 +235,11 @@ void retryRegeneratesMultiChunkBodyAndTrailers() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); - // Concatenated interior chunks (in order) plus the regenerated trailer, on the re-sent attempt. + // Concatenated interior chunks (in order) plus the reproduced trailer, on the re-sent attempt. assertThat(res.contentUtf8()).isEqualTo("abc|v"); // Deterministic: initial + one retry. Exact assertion guards against over-regeneration. assertThat(serverHits).hasValue(2); @@ -268,13 +268,13 @@ void factoryThrowingOnRetryFailsFast() { // The surfaced failure must carry the factory's own exception, not merely be "some Exception": // a connection error, timeout, or unrelated bug would also satisfy isInstanceOf(Exception.class). - assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() .isInstanceOf(IllegalStateException.class) .hasMessageContaining("cannot regenerate body"); - // The factory is consulted twice: once for the initial body (via defer()), once for the + // The factory is consulted twice: once for the initial body (via reproducible()), once for the // failing retry. It fails fast rather than looping through the whole retry budget. assertThat(bodyCalls).hasValue(2); } @@ -296,7 +296,7 @@ void followsRedirectRegeneratingBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -323,7 +323,7 @@ void seeOtherRedirectDropsBody() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); // On a 303 the method is rewritten to GET and the body dropped; the duplicator is aborted @@ -346,7 +346,7 @@ void stackedRetryAndRedirect() { return StreamMessage.of(HttpData.ofUtf8("redir-body")); }; - // Both RetryingClient and RedirectingClient present; the deferred body must be resent + // Both RetryingClient and RedirectingClient present; the reproducible body must be resent // correctly through the redirect without a destructive double-consume. final WebClient client = WebClient.builder(server.httpUri()) @@ -356,7 +356,7 @@ void stackedRetryAndRedirect() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -388,7 +388,7 @@ void factoryThrowingOnRedirectHopFailsFast() { .followRedirects() .build(); - assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -412,7 +412,7 @@ void directConsumeWithoutDecoratorSendsBodyOnce() { final WebClient client = WebClient.of(server.httpUri()); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); @@ -432,7 +432,7 @@ void directConsumeSurfacesThrowingFactory() { }; final WebClient client = WebClient.of(server.httpUri()); - assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -450,7 +450,7 @@ void directConsumeSurfacesNullFactory() { final Supplier> bodyFactory = () -> null; final WebClient client = WebClient.of(server.httpUri()); - assertThatThrownBy(() -> client.execute(HttpRequest.defer(headers, bodyFactory), + assertThatThrownBy(() -> client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join()) .getRootCause() @@ -459,7 +459,7 @@ void directConsumeSurfacesNullFactory() { @Test void toDuplicatorIgnoresMaxRequestLength() { - // The deferred duplicator never buffers, so it must ignore the maxRequestLength cap that a + // The reproducible duplicator never buffers, so it must ignore the maxRequestLength cap that a // buffering DefaultStreamMessageDuplicator would enforce. A body far larger than a tiny cap must // still stream to completion; a regression that fell back to a buffering duplicator would throw // ContentTooLargeException here. @@ -470,7 +470,7 @@ void toDuplicatorIgnoresMaxRequestLength() { final EventExecutor executor = CommonPools.workerGroup().next(); final HttpRequestDuplicator duplicator = - HttpRequest.defer(headers, bodyFactory).toDuplicator(executor, 8); + HttpRequest.reproducible(headers, bodyFactory).toDuplicator(executor, 8); final AggregatedHttpRequest produced = duplicator.duplicate().aggregate().join(); assertThat(produced.content().length()).isEqualTo(large.length); @@ -478,9 +478,9 @@ void toDuplicatorIgnoresMaxRequestLength() { } @Test - void basePathPrefixRemainsDeferred() { + void basePathPrefixRemainsReproducible() { // A WebClient built with a base-URI path prefix rewrites the request path via - // req.withHeaders(...). If DeferredHttpRequest did not override withHeaders, the rewritten + // req.withHeaders(...). If ReproducibleHttpRequest did not override withHeaders, the rewritten // request would be a plain HeaderOverridingHttpRequest whose toDuplicator falls back to the // buffering DefaultStreamMessageDuplicator — silently reintroducing the ~2 GiB limit. This test // pins that the rewritten request still regenerates its body per attempt (non-buffering path). @@ -502,13 +502,13 @@ void basePathPrefixRemainsDeferred() { .build(); final AggregatedHttpResponse res = - client.execute(HttpRequest.defer(headers, bodyFactory), streamingOptions()) + client.execute(HttpRequest.reproducible(headers, bodyFactory), streamingOptions()) .aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentUtf8()).isEqualTo("prefixed-body"); // Regenerated for the initial attempt and the retry — proving the path-rewritten request kept - // the deferred (non-buffering) duplicator rather than falling back to buffering. + // the reproducible (non-buffering) duplicator rather than falling back to buffering. assertThat(bodyCalls).hasValue(2); } } diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java similarity index 97% rename from core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java rename to core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java index f5b9e7d067a..f3c982f372b 100644 --- a/core/src/test/java/com/linecorp/armeria/client/retry/DeferredHttpRequestRetryTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/retry/ReproducibleHttpRequestRetryTest.java @@ -41,7 +41,7 @@ import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.testing.junit5.server.ServerExtension; -class DeferredHttpRequestRetryTest { +class ReproducibleHttpRequestRetryTest { @RegisterExtension static final ServerExtension server = new ServerExtension() { @@ -92,7 +92,7 @@ void retriesAfterFirstAttemptBodyFailsMidStream() { .exchangeType(ExchangeType.REQUEST_STREAMING) .build(); - final HttpRequest req = HttpRequest.defer(headers, bodySupplier); + final HttpRequest req = HttpRequest.reproducible(headers, bodySupplier); final AggregatedHttpResponse res = client.execute(req, options).aggregate().join(); assertThat(res.status()).isEqualTo(HttpStatus.OK); diff --git a/core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java similarity index 87% rename from core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java rename to core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java index cd40ea09114..bb2f2cee31c 100644 --- a/core/src/test/java/com/linecorp/armeria/common/DeferredHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java @@ -36,7 +36,7 @@ import com.linecorp.armeria.common.stream.StreamMessage; -class DeferredHttpRequestDuplicatorTest { +class ReproducibleHttpRequestDuplicatorTest { private static final RequestHeaders HEADERS = RequestHeaders.of(HttpMethod.POST, "/upload"); @@ -47,8 +47,8 @@ void everyDuplicateProducesAFreshBody() { calls.incrementAndGet(); return StreamMessage.of(HttpData.ofUtf8("body")); }; - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); // Even the first duplicate() invokes the factory — the caller's request is never reused. final HttpRequest first = dup.duplicate(); @@ -63,8 +63,8 @@ void everyDuplicateProducesAFreshBody() { void duplicateWithHeadersOverridesHeaders() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); final RequestHeaders overridden = RequestHeaders.of(HttpMethod.POST, "/upload", "x-attempt", "1"); final HttpRequest req = dup.duplicate(overridden); @@ -74,21 +74,21 @@ void duplicateWithHeadersOverridesHeaders() { @Test void factoryThrowingPropagates() { final Supplier> factory = () -> { - throw new IllegalStateException("cannot regenerate body"); + throw new IllegalStateException("cannot reproduce body"); }; - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); // Fail fast: the exception propagates so the client can terminate the request. assertThatThrownBy(dup::duplicate).isInstanceOf(IllegalStateException.class) - .hasMessageContaining("cannot regenerate body"); + .hasMessageContaining("cannot reproduce body"); } @Test void factoryReturningNullThrows() { final Supplier> factory = () -> null; - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); assertThatThrownBy(dup::duplicate).isInstanceOf(NullPointerException.class); } @@ -97,8 +97,8 @@ void factoryReturningNullThrows() { void duplicateAfterCloseThrows() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); dup.duplicate(); dup.close(); @@ -117,8 +117,8 @@ void duplicateAfterAbortTearsDownProducedBodyWithCause() { produced.add(body); return body; }; - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); final RuntimeException cause = new RuntimeException("cleanup"); dup.abort(cause); @@ -156,8 +156,8 @@ void concurrentAbortDuringDuplicateTearsDownProducedBody() throws Exception { } return body; }; - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); final RuntimeException cause = new RuntimeException("cleanup"); final ExecutorService executor = Executors.newSingleThreadExecutor(); @@ -191,8 +191,8 @@ void concurrentAbortDuringDuplicateTearsDownProducedBody() throws Exception { void abortReleasesAllOutstandingUnsubscribedRequests() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); // Multiple produced requests may be outstanding at once (e.g. hedging); abort() must tear // down every one of them, not just the most recently produced, so no body is leaked. @@ -208,8 +208,8 @@ void abortReleasesAllOutstandingUnsubscribedRequests() { void closeLeavesOutstandingRequestsActive() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); final HttpRequest produced = dup.duplicate(); // StreamMessageDuplicator contract: close() prevents further duplication but must not abort @@ -222,8 +222,8 @@ void closeLeavesOutstandingRequestsActive() { void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { final Supplier> factory = () -> StreamMessage.of(HttpData.ofUtf8("body")); - final DeferredHttpRequestDuplicator dup = - new DeferredHttpRequestDuplicator(HEADERS, factory); + final ReproducibleHttpRequestDuplicator dup = + new ReproducibleHttpRequestDuplicator(HEADERS, factory); // Draining a produced request completes it; it is then removed from the tracked set so the set // does not grow unbounded and a later abort() leaves the already-completed request untouched. From 7440a5e531f7d8dd9d879cf155627e89cdc2a69f Mon Sep 17 00:00:00 2001 From: Yizhou Feng Date: Tue, 11 Aug 2026 19:57:35 +0000 Subject: [PATCH 19/19] test: deflake completedRequestIsUntracked by joining whenComplete before asserting The BlockHound CI profile hit a latent race: after `aggregate().join()` returns, the request's own `whenComplete()` callback can fire slightly later on the event loop, so `assertThat(whenComplete()).isCompleted()` occasionally saw an incomplete future. Join `whenComplete()` first to make the assertion deterministic, matching the join-based pattern used elsewhere in the file. Co-authored-by: Isaac --- .../armeria/common/ReproducibleHttpRequestDuplicatorTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java index bb2f2cee31c..010325f86d5 100644 --- a/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/ReproducibleHttpRequestDuplicatorTest.java @@ -229,6 +229,10 @@ void completedRequestIsUntrackedSoAbortDoesNotAffectIt() { // does not grow unbounded and a later abort() leaves the already-completed request untouched. final HttpRequest produced = dup.duplicate(); produced.aggregate().join(); + // Wait for the request's own completion, which may resolve slightly after the aggregate future + // (the completion callback fires on the event loop); join() makes this deterministic rather + // than asserting on a completion that may not have fired yet. + produced.whenComplete().join(); assertThat(produced.whenComplete()).isCompleted(); dup.abort(new RuntimeException("cleanup"));