Activity

1. sergey-morenets commented on Dec 9, 2024

   

   sergey-morenets

   on Dec 9, 2024

   Author

   More actions

   Our deeper analysis shown that this regression appeared in 13.3 and possible reason is the replacement

   httpResponse = clientForRequest.send(httpRequest, BodyHandlers.ofByteArray());

   with

   httpResponse = clientForRequest.send(httpRequest, BodyHandlers.ofInputStream());

   in Http2Client.
2. sergey-morenets commented on Dec 10, 2024

   

   sergey-morenets

   on Dec 10, 2024

   Author

   More actions

   So IOException actually happens then we try to read response body (in IOUtils.toString):

   var response = cityClient.create(city);
   var body = IOUtils.toString(response.body().asInputStream(), Charset.defaultCharset());
   

   And main issue is that this exception occurs outside of Feign client and that means retry logic would not be executed.
3. hdfg159 commented on Dec 27, 2024

   

   hdfg159

   on Dec 27, 2024

   Last edited by hdfg159

   Contributor

   More actions

   If cityClient.create(city) returns a feign.Response, it should indeed handle the inputStream related exceptions by itself.

   If you want to reproduce the previous logic, the return type should be filled with byte[].
   eg: cityClient

   byte[] create(var city)
4. sergey-morenets commented on Dec 28, 2024

   

   sergey-morenets

   on Dec 28, 2024

   Author

   More actions

   Hi @hdfg159

   I'm fine with that but it's breaking change and should be mentioned in release notes and documentation.
5. hdfg159 commented on Dec 28, 2024

   

   hdfg159

   on Dec 28, 2024

   Contributor

   More actions

   Hi @hdfg159

   I'm fine with that but it's breaking change and should be mentioned in release notes and documentation.

   This should be a bug -> #2419 . If it's version 13.1, then using other clients(eg: apache httpclient or okhttp3) should yield different expected results.
6. hdfg159 commented on Dec 28, 2024

   

   hdfg159

   on Dec 28, 2024

   Contributor

   More actions

   If cityClient.create(city) returns a feign.Response, it should indeed handle the inputStream related exceptions by itself.

   If you want to reproduce the previous logic, the return type should be filled with byte[]. eg: cityClient

   byte[] create(var city)

   #1474 (comment)

   👍React with 👍1Reacted by Kevin Davis
7. velo

   added

   bugUnexpected or incorrect behaviorUnexpected or incorrect behavior

   regressionBugs and issues related to unintended breaking changesBugs and issues related to unintended breaking changes

   on Jun 18, 2026
8. velo commented on Jun 18, 2026

   

   velo

   on Jun 18, 2026

   Member

   More actions

   Thanks for narrowing this to the Http2Client change from BodyHandlers.ofByteArray() to BodyHandlers.ofInputStream() in 13.3. That makes this useful as a regression candidate, especially because the exception now escapes after the Feign invocation when the caller reads the response body.

   A compatible fix needs to preserve existing streaming behavior while restoring the expected retry/exception handling for malformed responses. I would gladly review a PR that adds a focused regression test around the WireMock malformed chunk case and keeps the public API unchanged.
9. seonwooj0810 commented on Aug 5, 2026

   

   seonwooj0810

   on Aug 5, 2026

   Contributor

   More actions

   Dug into this a bit more since @velo asked for a focused regression test — wanted to share what I found before opening a PR, because the root cause and the "correct" fix are more entangled than they look.

   Where the exception actually gets (mis)classified

   The retry loop only retries RetryableException:

   // SynchronousMethodHandler.runWithRetry
   try {
     return executeAndDecode(invocation, options);
   } catch (RetryableException e) {
     retryer.continueOrPropagate(e);
     ...
   }

   executeAndDecode wraps client.execute() IOExceptions via FeignException.errorExecuting(...) → RetryableException. But once client.execute() returns successfully, decoding happens in InvocationContext.decode():

   } catch (IOException e) {
     throw errorReading(response.request(), response, e);
   }

   FeignException.errorReading(...) returns a plain FeignException, not a RetryableException, so it never reaches the retry catch block. Before #2420 switched Http2Client from BodyHandlers.ofByteArray() to BodyHandlers.ofInputStream(), the chunked-encoding validation happened during clientForRequest.send(...), i.e. inside execute(), so it went through errorExecuting and was retryable. After #2420, that same validation is deferred until the decoder actually reads the (now lazy) InputStream, which routes through errorReading instead. Same underlying IOException, different classification, purely because of when the bytes get validated — that's the regression.

   Why the "obvious" fix isn't safe

   The naive fix — make errorReading produce a RetryableException too — directly conflicts with an existing, deliberately-named test:

   // FeignTest.java
   @Test
   void doesntRetryAfterResponseIsSent() throws Exception {
     ...
     Throwable exception = assertThrows(FeignException.class, () -> api.post());
     ...
   }

   That test (and its name) encodes an intentional principle: once a response has come back from the server, Feign won't auto-retry on a subsequent decode failure, presumably because the server may have already applied a non-idempotent side effect, and blindly retrying at that point isn't safe in general. Since InvocationContext.decode() can't distinguish "transport truncated the stream" from "a custom Decoder threw IOException for its own reasons," making all errorReading IOExceptions retryable would quietly reverse that safety principle for every Client/Decoder, not just Http2Client + malformed chunked encoding.

   A narrower option that doesn't touch that principle

   InvocationContext already has precedent for a size-bounded eager-read: disconnectResponseBodyIfNeeded buffers raw Response bodies up to 8KB before handing them back, specifically so small bodies behave synchronously. The same idea applied to Http2Client itself — eagerly read (and validate) small/typical response bodies (e.g. via BodyHandlers.ofByteArray() under a size threshold, falling back to ofInputStream() above it) — would restore the pre-#2420 retry behavior for the common "small JSON API response" case reported here, without reintroducing the large-body memory issue #2420 was fixing, and without changing errorReading's classification for everyone else.

   Happy to put together a PR for that narrower version (Http2Client-scoped, size-bounded eager read + a regression test reproducing this issue's WireMock malformed-chunk case) if that direction sounds right — didn't want to send a patch that either quietly reverts #2420 or contradicts doesntRetryAfterResponseIsSent without checking first. cc @hdfg159 since you authored #2420 and know the original memory-usage motivation best.