feat!: make feign.Request.Body streaming-ready#3360
Conversation
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
| jacksonJaxbJsonProvider.writeTo( | ||
| object, bodyType.getClass(), null, null, APPLICATION_JSON_TYPE, null, outputStream); | ||
| template.body(outputStream.toByteArray(), Charset.defaultCharset()); | ||
| template.body(Request.Body.of(outputStream.toByteArray())); |
There was a problem hiding this comment.
Instead of creating a temporary byte array, could we do something like this?
template.body( Request.Body.of(os -> jacksonJaxbJsonProvider.writeTo(
object, bodyType.getClass(), null, null, APPLICATION_JSON_TYPE, null, os) ) );
This would require a Body.of method that would take an interface that can write body content to an arbitrary stream, so I'm thinking that maybe what I'm suggesting would be some future improvement? If so, then I understand why it is done this way for now.
There was a problem hiding this comment.
While I like the elegance of that suggestion, I find three reasons to keep it as is:
- In vast majority of cases, XML/JSON encoders operate with small chunk of data fitting in-memory.
- It appears that some clients, servers and the
Loggermight benefit fromContent-Lengthheader. And to calculate the length, the data should be eagerly encoded into tangible bytes one way or another. - If there's retry feature enabled,
jacksonJaxbJsonProvider.writeTowill be executed two or more times which is not efficient.
Additionally, I like your idea from the other thread to add support for raw OutputStream and Writer bodies (not implemented in this PR). If there's a really big piece of XML or JSON document, users could leverage raw body supplier to stream the large payload:
interface Client {
@RequestLine("POST /api/v1/send")
@Header("Content-Type: application/json; charset=UTF-8")
void send(ThrowingConsumer<OutputStream, IOException> body);
}
client.send(outputStream -> jacksonJaxbJsonProvider.writeTo(..., outputStream));Alternatively, users can provide custom Encoder implementation with lazy encoding configured.
I'd be happy to hear maintainers' opinion too, and I'm open to any idea.
TL;DR I prefer to keep existing encoders as is and let users enable streaming on demand if needed.
There was a problem hiding this comment.
I agree with you about most json encoders working on relatively small chunks. And I agree that there is really no need to make changes to the existing encoders.
You make a very good point about logging - Feign users rely on body logging, and having the body content in memory makes logging a lot simpler. I actually explored logging with streams in my initial prototypes for this project using buffered streams and mark/reset, but it was overly complicated and did not add sufficient value. Your design is much better.
Note that there is a "streaming" encoder for Feign that looks like it is designed to handle very large json streams, and they might benefit from this (I haven't dug into that encoder in depth), but for the general case, I am in agreement that we should leave the existing encoders alone until there is a compelling reason to change them.
|
I've read over the core changes, and I had one smallish input on this change: "Removed feign.RequestTemplate#charset instance variable — charset should be read from Content-Type headers." I think that this should be carefully considered from a backwards compatibility viewpoint. Some REST servers do not properly set the charset header - in those cases, it is probably important that the Feign user be able to set an override using the @headers annotation. I have not reviewed the code in sufficient detail to know whether this type of override is still possible, but that should probably be checked. We should also be aware that existing Feign users that rely on the old default UTF-8 content type behavior (for example, when interacting with servers that send the wrong content type header in the response), could have problems. I don't really see a good way around this. Personally, I think that this is a breaking change that we should be ok with - the library should have always used the headers for content type determination, and it is better to fix this now, even if it introduced breaking changes for some misconfigured servers. |
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
Co-authored-by: trumpetinc <6618744+trumpetinc@users.noreply.github.com>
I believe this is out of #2734 scope. From my experience, there's only a handful of cases where the client might not want to trust the server's If we're talking about the scenario where the server doesn't return |
Co-authored-by: trumpetinc 6618744+trumpetinc@users.noreply.github.com
Summary
This PR is the first step toward resolving #2734 — enabling request body streaming in Feign. It builds on the foundational work from @trumpetinc in #2754, which had become too outdated to merge directly.
The core idea is to change
feign.Request.Bodyfrom abyte[]-backed structure to a streaming-ready abstraction. This means request bodies are no longer cached in memory unless explicitly needed — enabling Feign to send large files and streams without buffering.Important
This change does not introduce public streaming API in Feign. It only makes the Feign client streaming-ready. Public streaming API is planned to be implemented in a follow-up PR.
Warning
This is a breaking change and is intended to be released as
v14.rc.1.What changed
Core changes
feign.Request.Bodyis now an interface with:writeTo(OutputStream)— the primary write mechanismcontentLength()— returns-1for unknown/streaming bodiesisRepeatable()—falsefor non-repeatable bodieswriteToByteArray()andwriteToString(Charset)— helper methods for repeatable bodies (use with caution)Body.of(...)— factory methods forString,byte[]inputsfeign.Request.BodyImplis the default implementation for repeatable, non-streaming bodies. It overridestoString()for human-readable logging.feign.RequestTemplatenow exposes a singlebody(Request.Body)setter. The oldbody(byte[], Charset)overload is@Deprecatedfor backward compatibility withspring-cloud-openfeign-core.feign.Request.body()now returnsOptional<Request.Body>instead ofbyte[].feign.Request#length()removed — useRequest.Body#contentLength()instead.feign.RequestTemplate#requestBody()returnsOptional<Request.Body>.feign.RequestTemplate#charsetinstance variable — charset should be read fromContent-Typeheaders.HTTP client updates
All
feign.Clientimplementations updated to stream bodies viaRequest.Body#writeTo:DefaultClientbody.get().writeTo(out)ApacheHttp5ClientFeignBodyEntitywrappingRequest.BodyAsyncApacheHttp5ClientClassicRequestBuilder+ClassicToAsyncRequestProducerfor true streamingOkHttpClientRequestBodydelegating tofeign.Request.BodyHttp2ClientBodyPublishers.ofInputStreamviaPipedInputStream/PipedOutputStreamGoogleHttpClientFeignBodyContentinner classJAXRSClientStreamingOutputVertxHttpClientOutputToReadStreambridge (see below)Encoder updates
All encoder implementations updated to call
template.body(Request.Body.of(...)):GsonEncoder,Jackson3Encoder,JAXBEncoder,JacksonJaxbJsonEncoder,Fastjson2Encoder,MoshiEncoder,SOAPEncoder,DefaultEncoder,JsonEncoder,Fastjson2EncoderVert.x streaming bridge
A new
feign.vertx.OutputToReadStreamclass bridges Java blockingOutputStreamto Vert.xReadStream<Buffer>. It is adapted fromio.cloudonix:vertx-java.io(MIT License) with a local compatibility fix to support both Vert.x 4 and Vert.x 5 (usingonCompleteinstead ofandThento avoid a runtime linkage toio.vertx.core.Completableabsent in Vert.x 4).An issue has been filed upstream: cloudonix/vertx-java.io#8. Once fixed upstream,
OutputToReadStreamcan be removed from this repo.VertxFeign.Buildernow requires.vertx(Vertx)in addition to.webClient(WebClient). ANullPointerExceptionwith a descriptive message is thrown if either is missing.FeignExceptionchangesFeignException.errorReading(...)now passesnullas the body (instead ofrequest.body()), since the request body may be a non-repeatable stream and should not be cached.isEmpty().Logging
Loggerupdated to useRequest.Body#toString()(provided byBodyImpl) for repeatable bodies.Metrics
dropwizard-metrics4/5MeteredEncoder: usesbody.contentLength().micrometerMeteredEncoder: readsContent-Lengthheader for recording.mockmoduleRequestKeyno longer stores or comparesCharset.RequestKey.Builder#charset(Charset)removed.Known limitations / future work
spring-cloud-openfeign-corecompatibility:RequestTemplate#body(byte[], Charset)kept as@Deprecated. Once the Spring team migrates tobody(Request.Body), this can be removed.Content-Typeheaders.UTF-8is used as a fallback inwriteToString. A separate issue/PR may be needed.FeignExceptiondesign: Response bodies are still cached asbyte[]inFeignException. Reconsidering this is deferred to a future PR.-Djapicmp.skip=trueproperty provided.Credits
Special thanks to @trumpetinc whose original #2754 laid the groundwork for this change.
Related
feign.form.MultipartFormContentProcessorstreamable (introduceclass MultipartBody implements feign.RequestBody)