-
Notifications
You must be signed in to change notification settings - Fork 1k
Reproducible request bodies #6841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
5128066
c9d269d
2a846e7
d0d1d0d
4ca6f45
c3928a1
866dc10
705aaf5
57ea25d
80de365
8860934
fa94f5c
f30c82c
1286461
1f88945
9592a79
8a66f3e
0cd4d64
7440a5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| * | ||
| * <p>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: | ||
| * | ||
| * <pre>{@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); | ||
| * }</pre> | ||
| * | ||
| * <p>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}. | ||
| * | ||
| * <p>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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I understood this is combining two concerns:
e.g. static HttpRequest of(RequestHeaders headers,
Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory) {
requireNonNull(headers, "headers");
requireNonNull(bodyFactory, "bodyFactory");
return of(headers, bodyFactory.get());
}
...
static HttpRequest reproducible(
RequestHeaders headers,
Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory) {
requireNonNull(headers, "headers");
requireNonNull(bodyFactory, "bodyFactory");
return of(headers, bodyFactory).withDuplicatorFactory(bodyFactory);
}For your use-case, do you see most users will prefer using I do think the single factory method has value in that users only need to specify a single
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — I'd prefer to keep the single |
||
| RequestHeaders headers, | ||
| Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory) { | ||
|
Comment on lines
+423
to
+425
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question) I'm unsure about the naming. What do you think of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @minwoox suggested that we keep the name
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea — renamed to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry but, could we revert to
What actually defines this type is the opposite constraint: each call must supply an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and reverted to |
||
| requireNonNull(headers, "headers"); | ||
| requireNonNull(bodyFactory, "bodyFactory"); | ||
| return new ReproducibleHttpRequest(headers, bodyFactory); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a new {@link HttpRequestBuilder}. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
| * | ||
| * <p>See {@link HttpRequest#reproducible(RequestHeaders, Supplier)} for details and usage. | ||
| * | ||
| * <p>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<HttpObject, HttpRequestDuplicator> implements HttpRequest { | ||
|
|
||
| private final RequestHeaders headers; | ||
| private final Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory; | ||
|
|
||
| ReproducibleHttpRequest(RequestHeaders headers, | ||
| Supplier<? extends StreamMessage<? extends HttpObject>> 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<HttpObject> lazyBody( | ||
| Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory) { | ||
| return StreamMessage.of((Publisher<HttpObject>) subscriber -> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question) Is it necessary that the supplier isn't called eagerly? Asking because from what I understand, If it is necessary for your use-case, I think it's fine to go ahead with this impl. I think it may be possible to generalize
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Laziness is necessary here: the caller's factory may have subscribe-time side effects (opening a file, acquiring a handle), and on the retry/redirect path the delegate is never subscribed —
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks
Feel free to go ahead, or we can handle this as well if you are busy 🙇 Let us know
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'd be great — please go ahead with the |
||
| final StreamMessage<? extends HttpObject> body; | ||
| try { | ||
| body = bodyFactory.get(); | ||
| } catch (Throwable t) { | ||
| StreamMessage.<HttpObject>aborted(t).subscribe(subscriber); | ||
| return; | ||
| } | ||
| if (body == null) { | ||
| StreamMessage.<HttpObject>aborted( | ||
| new NullPointerException("bodyFactory.get() returned null.")).subscribe(subscriber); | ||
| return; | ||
| } | ||
| @SuppressWarnings("unchecked") | ||
| final StreamMessage<HttpObject> cast = (StreamMessage<HttpObject>) body; | ||
| cast.subscribe(subscriber); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public RequestHeaders headers() { | ||
| return headers; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public CompletableFuture<AggregatedHttpRequest> 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); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
| * | ||
| * <p>{@code RetryingClient} and {@code RedirectingClient} drive this duplicator strictly | ||
| * sequentially: at most one produced request is outstanding at a time, and each access | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think it is possible that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is handled by the instance lock, and I've now added tests that pin it. The race — |
||
| * 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question) Does this have to be in the internal package? Can this class be in the common pkg with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — moved it to |
||
|
|
||
| private final RequestHeaders headers; | ||
| private final Supplier<? extends StreamMessage<? extends HttpObject>> bodyFactory; | ||
|
|
||
| @Nullable | ||
| private volatile HttpRequest lastProduced; | ||
| private volatile boolean closed; | ||
|
|
||
| public ReproducibleHttpRequestDuplicator( | ||
| RequestHeaders headers, | ||
| Supplier<? extends StreamMessage<? extends HttpObject>> 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I prefer that the semantics of This would also help with supporting hedging more easily in the future. Alternatively, I'm fine with keeping the current implementation and improving in the future as well as I do think complexity of the impl would go up slightly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's the contract of a duplicated request.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a factory creates a new child stream, it creates an independent stream, so each child may publish different elements. I don't think it will be used that way in most cases. That said, given the limitation of the current approach - that the upstream and downstream data may differ - I'm not sure it makes sense to close the downstream when the upstream is closed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The contract guarantees that it's the same stream: Unlike
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Summarizing where this landed: there's no shared upstream by design — each |
||
| 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<? extends HttpObject> 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(); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.