Fix Slf4jLogger and JavaLogger returning unbuffered response when logging disabled #3172
+408
−8
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@velo
Problem
Slf4jLoggerandJavaLoggercurrently behave differently depending on the logging level.When the SLF4J level is set to
INFOor higher (orjava.util.loggingis aboveFINE),logAndRebufferResponse()does not delegate tosuper.logAndRebufferResponse().As a result, the response body is not rebuffered and remains a one-time
InputStream.This leads to several practical issues:
ErrorDecoderimplementations cannot reliably read the response body from error responsesresponse.body().toString()returns an instance reference (e.g.feign@...) instead of the actual contentSolution
Always delegate to
super.logAndRebufferResponse(), regardless of the logger’s runtime level.This ensures that response buffering follows Feign’s
Logger.Levelconsistently and is no longer affected by SLF4J or JUL configuration differences.Result
Logger.Level >= HEADERS, regardless of SLF4J or JUL logging levelsErrorDecodercan reliably read response bodiesChanges
Slf4jLogger: Removed conditional logic inlogAndRebufferResponse()and always delegate tosuperBehavioral Impact
Memory usage
Logger.LevelisHEADERSorFULL, even if the underlying SLF4J / JUL level isINFOor higherAll existing tests pass.
I’m happy to adjust this approach if there are alternative suggestions or concerns from the maintainers.
Alternative Approach Considered
Option: Lazy buffering with
LazyBufferedBodyAs an alternative to eager buffering, we also considered introducing a
LazyBufferedBodywrapper that would:Logger.Level >= HEADERSErrorDecoder)This approach could reduce memory usage in some cases, especially when response bodies are rarely accessed.
The current implementation is not meant to assert that eager rebuffering is the only or best solution.
Rather, it serves as a concrete proposal to make the existing behavior consistent.
If maintainers prefer a different approach—such as lazy buffering—I’m happy to adjust the implementation based on that feedback.
Fixes #1336