Streaming decoder support2 by trumpetinc · Pull Request #3551 · OpenFeign/feign

trumpetinc · 2026-09-01T16:50:09Z

Adds support for streaming decode for Feign RequestLine methods that return InputStream or Reader.

To use:

  interface LargeStreamTestInterface {

    @RequestLine("GET /")
    InputStream getLargeStream();

    @RequestLine("GET /")
    Reader getLargeReader();
  }

public void test(){
  try(InputStream is = myLargeStreamTestInterface.getLargeStream()){
     // process the is
  }
}

Changes

InvocationContext now leaves the response stream open if the return type from the decoder implements Closeable
New InputStreamAndReaderDecoder class that can be registered with the Feign builder.decoder() method. Supports passing the decode request to a delegate if the method return type is not InputStream or Reader. If the return type is Reader, the charset of the response Content-Type header is used. If no charset is specified in the header, UTF-8 is assumed.
New ContentTypeParser utility method for obtaining information from the Content-Type header

trumpetinc · 2026-09-01T16:50:43Z

Fixing problems with initial git branch ( #3494 )

trumpetinc · 2026-09-01T16:55:55Z

@velo let's try this - I have no idea what I messed up with the git branching on the earlier PR. This PR is as clean as I can make it. I branched from the latest 14.x and made the required changes.

CircleCI is already complaining about build problems (certainly nothing to do with this PR itself??):

wget: Failed to fetch https://downloads.apache.org/maven/mvnd/1.0.2/maven-mvnd-1.0.2-linux-amd64.zip

But at least security snyk checked out this time.

trumpetinc · 2026-09-02T15:31:18Z

@velo this is ready for review. I implemented it as a separate decoder class - if you prefer that this functionality be implemented in DefaultDecoder instead, we can do that - but I'll need you to merge the latest PredicatedDecoder changes into the 14.x branch (DefaultDecoder in 14.x isn't a PredicatedDecoder).

velo

Thanks for this — streaming decoder support is a real gap. Two structural issues before this can merge, both of which also shrink the diff a lot once fixed:

InvocationContext.proceed() infers "don't close the response" from whether the declared return type is Closeable, on the single shared decode path every Feign call goes through. This duplicates a mechanism that already exists: BaseBuilder.doNotCloseAfterDecode(), explicitly documented for "lazy-evaluated constructs," where the custom decoder is responsible for closing. As written, any existing method that returns any Closeable-implementing type through any decoder (not just the new one) will silently stop having its response closed — a connection-leak risk with no opt-in and no test guarding it. It's also inconsistent with itself: wrap the same return type in TypedResponse<InputStream> and rawType is no longer Closeable, so the response gets closed anyway, breaking the exact case this was added for. Please drop the Closeable/noClose addition entirely and have streaming callers use the existing doNotCloseAfterDecode() flag, with the new decoder taking ownership of closing as the existing contract already requires of custom decoders.
ContentTypeParser.parseContentTypeHeader reimplements Response.charset() (same split-on-;-then-= logic, even the same contentTypeParmeters typo) as new public API (ContentTypeResult, with an unused getContentType()), and ships with a // TODO admitting it doesn't implement the full parser spec, with no dedicated test. StringDecoder already reuses Response.charset() for this. Please drop ContentTypeParser/ContentTypeResult and call response.charset() directly in the Reader branch. If quoted-charset support is a real gap, that's worth fixing in Response.charset() itself rather than a second parallel implementation.

Smaller cleanup while you're in there:

The blank-line-only change in DefaultDecoder.java looks like unintentional diff noise.
The .mvn/wrapper/maven-wrapper.properties mvnd URL fix looks like an unrelated fix that landed on this branch — worth splitting into its own PR so this one's diff stays reviewable on its own terms.
InputStreamAndReaderDecoderTest starts MockWebServer instances but doesn't stop them — worth adding @AfterEach cleanup.

Happy to take another look once the two structural points are addressed — the feature itself (an InputStream/Reader-capable decoder) is worth having, it just needs to lean on the existing extension points instead of adding parallel ones.

trumpetinc · 2026-09-08T16:44:45Z

@velo good feedback, thank you.

I could use your guidance on one of your requests: doNotCloseAfterDecode is a FeignBuilder method. I do not see how we can make a single decoder set that value. And given the move to MultiDecoder, I am certain that we don't want to force the user to have to create a separate Feign instance for streaming decoders vs others.

Potential approaches

Add a doNotCloseAfterDecode setting in Response (which would then pass to Request and be checked/honored in the InvocationContext). And the default value of that would pull from the Feign builder setting (maintaining backwards compatibility).

Concern with that is that Response is currently immutable. So we are talking about making it mutable - AND requiring a side-effect from the Decoder. Not great.

Make Decoder return a DecodeResult object that encapsulates the decoded Object, along with an autoClose argument.

Downside of this is that we have to change the method signature of Decoder. And because DecodeResult is itself an Object, we have to be a little careful to ensure there are no code paths that don't properly process the DecodeResult. I'm not overly concerned with this downside, but it does need to be considered.

Add a shouldAutoclose() method to the Decoder interface (with default method returning false).

Downside here is that this sort of approach has a way of really cluttering an interface spec. Over time, we wind up adding a bunch of boolean methods to an interface to control different aspects of the processing. Wearing my architecture hat, I am not crazy about this option - even though it is probably the least disruptive.

Add a getDecoderHints() method to the Decoder interface (with default method returning a default Hints instance). For now, DecoderHints would have a single shouldAutoClose() property, but we could add others without further cluttering the interface.

Preference

My preference would be option 2 - it is a better design overall. But it absolutely makes this a v14 change (breaking). So you may prefer option 3 or 4.

How you would like me to pursue this?

trumpetinc · 2026-09-08T22:13:46Z

@velo can you please get the change from master:/feign/.mvn/wrapper/maven-wrapper.properties merged into OpenFeign:14.x? That will be necessary for CI to run on this PR. Thanks.

trumpetinc · 2026-09-08T22:17:23Z

ContentType parser has been removed (I still think that centralizing parsing of content-type headers is eventually desirable, but that doesn't need to be done here).
DefaultDecoder blank line has been reverted - thanks for catching that
MockWebServer is now started and stopped using a try-with-resources block in each test

I have reverted the change to the maven config file - as soon as the mvn config change is merged from master to 14.x, the CI build will work again.

trumpetinc · 2026-09-21T14:49:04Z

@velo checking in to see if you have any feedback on:

#3551 (comment)
and
#3551 (comment)

velo · 2026-09-21T18:15:22Z

Option 5 — the Closeable check you already pushed — is the one I want. No API change, and it keys off the thing that actually determines ownership: if the caller receives a Closeable, the caller closes it.

Ruling out the others, briefly:

Option 2 is the better design in the abstract and I agree with your reasoning, but it breaks every Decoder in the ecosystem — third-party decoders, lambdas, all of them — to carry one boolean. And DecodeResult being itself an Object is a real footgun: a missed unwrap compiles cleanly and hands the wrapper back to the caller.

Options 3 and 4 have a problem that isn't obvious until you wire them up. InvocationContext holds a single Decoder, and under MultiDecoder that one is the multiplexer, not the decoder that actually ran. So shouldAutoclose() can't be no-arg — it has to take (Response, Type), and MultiDecoder has to re-resolve its routing to answer it, running canDecode a second time. That's a lot of machinery for one boolean.

Option 1 makes Response mutable and makes decoding side-effecting. Agreed, not great.

Two things to tidy up on the current implementation:

TypedResponse<InputStream> doesn't hit the check — rawType is TypedResponse, so noClose stays false and we close the stream out from under the caller. Either resolve through to the body type, or explicitly document that streaming and TypedResponse don't combine.
The flip side of the heuristic: a decoder returning a fully-buffered value whose type happens to implement Closeable will now leak the response body. I think that's the right trade — the caller got a Closeable and owns it — but it should be stated in the javadoc rather than left implicit.

If we ever do need a per-decoder override, option 4 is additive on top of this. Option 2 never can be.

On the maven wrapper: that one's already done. The mvnd 1.0.6 fix landed on 14.x on Sept 4 via #3557, and master was merged into 14.x on Sept 9 (9c210ad2) — both branches have identical .mvn/wrapper/maven-wrapper.properties now. The failing pr-build on this PR is from Sept 8, before that merge landed. Rebase onto current 14.x and CI should run.

not in this PR.

trumpetinc · 2026-09-22T18:26:57Z

@velo I rebased this PR from 14.x, but it's now showing a massive number of commits and file changes from other merges. I thought I understood git, but I must be doing something really wrong. I'll start over. rgggg. Sorry for the delay.

not in this PR.

https://github.com/trumpetinc/feign.git into streaming_decoder_support2

trumpetinc · 2026-09-22T23:04:22Z

@velo ok - this is ready for you - sorry for the delay.

I have left the original approach as-is (type check on Closable), but added additional handling and tests for TypedResponse with streaming bodies.

I think we are now in agreement about the functional implementation.

Next, how do you want to handle the structural implementation?

Options:

Leave it as I have it (streaming decoding is done by a dedicated Decoder that must be included in the decoding chain (presumably registered using builder.decoders(PredicatedDecoder...)).

or

Add InputStream and Reader decoding to DefaultDecoder (mirroring the addition of standard streaming encoders to DefaultEncoder). And if we go this route, should I remove InputStreamAndReaderDecoder entirely?

trumpetinc · 2026-09-23T13:40:31Z

@velo

The flip side of the heuristic: a decoder returning a fully-buffered value whose type happens to implement Closeable will now leak the response body. I think that's the right trade — the caller got a Closeable and owns it — but it should be stated in the javadoc rather than left implicit.

I added a section to the bottom of the MIGRATION-v14.md document that covers this (and includes examples of how to create custom streaming decoders).

I added javadoc to Decoder explaining that the caller is responsible for closing the returned stream.

I added javadoc to InputStreamAndReaderDecoder explaining that the caller is responsible for closing the returned stream.

Do these cover what you had in mind, or do we need something in the README.md file? I'm assuming that at some point some content from MIGRATION-v14.md is going to make it's way into README.md...

trumpetinc · 2026-09-23T13:44:21Z

Remaining open item:

Structural question: Streaming decoder support2 #3551 (comment)

velo

Thanks for the TypedResponse handling and the docs. Here's my answer on the structural question, plus one regression to fix.

Structure: fold it into `DefaultDecoder`

Go with option 2. DefaultEncoder already handles File/Path/InputStream, so the decoder should match. InputStream foo() should work on a plain Feign.builder() without users having to find and register a separate class. DefaultDecoder throws on these types today, so this only adds behaviour. Please remove InputStreamAndReaderDecoder so there's only one way to do it. If you need DefaultDecoder to be a PredicatedDecoder on 14.x first, tell me and I'll merge master into 14.x.

Blocking: `void` bodies are no longer closed when `doNotCloseAfterDecode()` is set

This PR removes ensureClosed(response.body()) from the isVoidType(returnType) && !decodeVoid branch. With the default closeAfterDecode = true the finally block still closes the body, so nothing changes there. With doNotCloseAfterDecode(), though, shouldClose starts out false, and every void call now leaks its connection. Before this PR that branch always closed the body. Please restore that line and add a test with doNotCloseAfterDecode() plus a void method.

Smaller items

In proceed(), the TypedResponse path checks bodyType instanceof Class<?> while the plain path uses Types.getRawType. Use Closeable.class.isAssignableFrom(Types.getRawType(bodyType)) in both places so they behave the same.
Rename rslt to result, and drop the blank line before } finally {.
MIGRATION-v14.md:
- "Closable" → Closeable (appears 3 times, including the implements Closable code sample).
- The example declares ExampleInterface but targets LargeStreamTestInterface, and the decoder returns new ExampleStream(...) where it should be ExampleStreamingReturn.
- The first code block mixes tabs with trailing whitespace.
- "callers responsibility" → "caller's responsibility".
- The file has no newline at the end.
- Once the decoder lives in DefaultDecoder, update the example to drop .decoders(new InputStreamAndReaderDecoder(), ...).
A short section in README.md on returning InputStream/Reader (and that the caller must close it) would help. The migration guide covers what changed, but new users read the README.

The Decoder javadoc note and the migration section are enough to cover the "buffered Closeable now leaks" trade-off. Thanks!

trumpetinc · 2026-09-23T20:02:41Z

Void types are back to force-closing, regardless of doNotCloseAfterDecode() - I see why that is important (there's nothing for the caller to close responsibly when we return void) - apologies for not thinking about that.

Question: The original code does not call close when doNotCloseAfterDecode and decodeVoid are both active. In this situation, we are returning void so there is no opportunity for the caller to close. So it seems like we should be closing ALL void return types, regardless of doNotCloseAfterDecode or decodeVoid. I have implemented the logic this way and added tests - please let me know if I'm missing something.

Tests added to FeignTest:

voidMethodsCloseBodyWhenDoNotCloseAfterDecodeIsActive
voidMethodsCloseBodyWhenDoNotCloseAfterDecodeAndDecodeVoidIsActive
closableMethodsDoNotCloseBody

Other changes:

instanceof checks now use Types.getRawType() consistently
rslt -> result renamed, and blank line removed
Closable -> Closeable (USA English spelling mistake!)
Migration-v14.md
- class names in example have been made consistent
- tabs in code examples have been converted to double spaces
- newline added to end of file
- "callers responsibility" → "caller's responsibility"

As time allows, I will move the functionality into the DefaultDecoder and remove InputStreamAndReaderDecoder, adjust tests, and update documentation.

the body. Fix documentation issues. Fix void type return issues.

trumpetinc · 2026-09-23T22:01:00Z

@velo

Streaming decoding has been moved into DefaultDecoder (and that is already a PredicatedDecoder through StringDecoder, so I think that was merged earlier). All tests and documentation have been updated.

I added a short section to README.md titled ### Streaming Responses

I believe that completes all punchlist items. Please let me know if there is anything else!

velo

Thanks for working through the last round. Nearly all of it is addressed. Remaining items:

Blocking

The close decision uses the declared return type, not the returned value (InvocationContext.proceed(), the Closeable.class.isAssignableFrom(rawType) check). When the decoder returns null for a Closeable return type, the caller has nothing to close and the body leaks. For example: InputStream download() with .dismiss404() and a 404 with an HTML body. DefaultDecoder returns emptyValueOf(InputStream.class), which is null, so the connection is never closed. The same applies to TypedResponse<InputStream> and to custom decoders that return null. closableMethodsDoNotCloseBody currently asserts this leak. Please base the decision on the result (e.g. !(result instanceof Closeable), or check result.body() for TypedResponse), and add a test for the 404 + dismiss404 + InputStream case.
Commit 91a1b61 has no test showing that the body is closed when decode throws for a Closeable return type.

Docs

MIGRATION-v14.md ~L449 still has LargeStreamTestInterface api = Feign.builder().target(ExampleInterface.class, ...).
The same code block still has trailing whitespace (~L448, L454).
README.md:120 says "Closable"; it should be "Closeable".
Please mention in the README that Logger.Level.FULL buffers the response, which turns streaming off.

Nits

The Decoder javadoc says "Closeable values", but the behaviour is keyed on the declared type. This goes away once the blocking item is fixed.
isResponseBodyClosed() matches "closed" in the exception message, which is fragile.
The streaming tests use new Random() without a seed.

trumpetinc mentioned this pull request Sep 1, 2026

Add streaming decoder support #3494

Closed

trumpetinc requested a review from velo September 1, 2026 18:26

velo requested changes Sep 4, 2026

View reviewed changes

trumpetinc added 10 commits September 22, 2026 11:01

Add streaming decoders

4065b5a

code formatting

0f3c444

Fix formatting

4e479c7

Make InputStreamAndReaderDecoder a PredicatedDecoder

d98a3cd

Fix formatting

7321620

Make sure mock web server gets closed

f6750bc

Parse charset using existing code

93d299f

Kill accidental diff

7dbed76

Revert change - it should come in from a merge from master into 14.x,

1a8fa04

not in this PR.

Fix formatting

05bf465

trumpetinc added 3 commits September 22, 2026 15:08

Apache download no longer hosting mvnd 1.0.2 - moving to 1.0.6

d9be32f

Revert change - it should come in from a merge from master into 14.x,

7c406d2

not in this PR.

Merge branch 'streaming_decoder_support2' of

41b4581

https://github.com/trumpetinc/feign.git into streaming_decoder_support2

trumpetinc force-pushed the streaming_decoder_support2 branch from aa3ac9e to 41b4581 Compare September 22, 2026 21:39

trumpetinc added 2 commits September 22, 2026 16:52

Handle streaming TypedResponses

bbf0cc0

Fix formatting

d32f339

Fix scenario where decoding throws exception leaves the body unclosed.

91a1b61

trumpetinc added 3 commits September 23, 2026 07:21

Document breaking changes in migration-v14 documentation

f6f53b5

Add javadoc

b3dd854

Add javadoc about Decoders for Closeable return types

c970cca

formatting

0e25883

formatting

f2047e4

velo requested changes Sep 23, 2026

View reviewed changes

trumpetinc added 2 commits September 23, 2026 15:36

Add tests for void returns and ensure closeable return types don't close

0c7d030

the body. Fix documentation issues. Fix void type return issues.

Move InputStreamAndReaderDecoder functionality into DefaultDecoder

cdbd2e0

velo reviewed Oct 2, 2026

View reviewed changes

Conversation

trumpetinc commented Sep 1, 2026

Changes

Uh oh!

trumpetinc commented Sep 1, 2026

Uh oh!

trumpetinc commented Sep 1, 2026

Uh oh!

trumpetinc commented Sep 2, 2026

Uh oh!

velo left a comment

Choose a reason for hiding this comment

Uh oh!

trumpetinc commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Potential approaches

Preference

Uh oh!

trumpetinc commented Sep 8, 2026

Uh oh!

trumpetinc commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trumpetinc commented Sep 21, 2026

Uh oh!

velo commented Sep 21, 2026

Uh oh!

trumpetinc commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trumpetinc commented Sep 22, 2026

Uh oh!

trumpetinc commented Sep 23, 2026

Uh oh!

trumpetinc commented Sep 23, 2026

Uh oh!

velo left a comment

Choose a reason for hiding this comment

Structure: fold it into DefaultDecoder

Blocking: void bodies are no longer closed when doNotCloseAfterDecode() is set

Smaller items

Uh oh!

trumpetinc commented Sep 23, 2026

Uh oh!

trumpetinc commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

velo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

trumpetinc commented Sep 8, 2026 •

edited

Loading

trumpetinc commented Sep 8, 2026 •

edited

Loading

trumpetinc commented Sep 22, 2026 •

edited

Loading

Structure: fold it into `DefaultDecoder`

Blocking: `void` bodies are no longer closed when `doNotCloseAfterDecode()` is set

trumpetinc commented Sep 23, 2026 •

edited

Loading