fix: prevent duplicate cache error handler execution in FeignCachingInvocationHandlerFactory#1363
Conversation
Signed-off-by: 98001yash <yashchauhan.gaya@gmail.com>
…nvocationHandlerFactory Closes spring-cloud#681 Signed-off-by: 98001yash <yashchauhan.gaya@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes duplicate cache error handling when using @Cacheable on Feign clients by attempting to prevent nested cache interception within FeignCachingInvocationHandlerFactory, and updates the OpenFeign documentation with additional caching notes.
Changes:
- Added a
ThreadLocalre-entrancy guard aroundCacheInterceptor.invoke(...)inFeignCachingInvocationHandlerFactory. - Added a “Limitations” note under Feign Caching docs.
- Added a new
FeignClientBuilderdocumentation section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCachingInvocationHandlerFactory.java |
Adds a ThreadLocal guard intended to prevent nested cache interception. |
docs/modules/ROOT/pages/spring-cloud-openfeign.adoc |
Adds caching limitation text and introduces a new FeignClientBuilder section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ✅ ADDED: Prevent nested cache invocation | ||
| if (CACHE_IN_PROGRESS.get()) { | ||
| return delegateHandler.invoke(proxy, method, args); | ||
| } | ||
|
|
||
| try { | ||
| CACHE_IN_PROGRESS.set(true); | ||
|
|
||
| return cacheInterceptor.invoke(new MethodInvocation() { |
There was a problem hiding this comment.
The ThreadLocal guard is only set inside this invocation handler, so it won’t be active when caching is initiated outside Feign (e.g., when a Spring AOP CacheInterceptor wraps the Feign proxy, as described in #681). In that flow this handler will still call cacheInterceptor.invoke(...), so the duplicate cache interception / error-handler execution would remain. Consider a mechanism that detects an already-active Spring Cache invocation (or prevents Feign caching from applying when Spring caching already proxies the client), and add a regression test that asserts cache get errors are handled only once.
| @@ -50,32 +53,45 @@ public InvocationHandler create(Target target, Map<Method, MethodHandler> dispat | |||
| final InvocationHandler delegateHandler = delegateFactory.create(target, dispatch); | |||
| return (proxy, method, argsNullable) -> { | |||
| Object[] args = Optional.ofNullable(argsNullable).orElseGet(() -> new Object[0]); | |||
| return cacheInterceptor.invoke(new MethodInvocation() { | |||
| @Override | |||
| public Method getMethod() { | |||
| return method; | |||
| } | |||
|
|
|||
| @Override | |||
| public Object[] getArguments() { | |||
| return args; | |||
| } | |||
|
|
|||
| @Override | |||
| public Object proceed() throws Throwable { | |||
| return delegateHandler.invoke(proxy, method, args); | |||
| } | |||
|
|
|||
| @Override | |||
| public Object getThis() { | |||
| return target; | |||
| } | |||
|
|
|||
| @Override | |||
| public AccessibleObject getStaticPart() { | |||
| return method; | |||
| } | |||
| }); | |||
|
|
|||
| // ✅ ADDED: Prevent nested cache invocation | |||
| if (CACHE_IN_PROGRESS.get()) { | |||
There was a problem hiding this comment.
Please remove the “ADDED” markers / emoji in comments. They aren’t used elsewhere in the codebase and will likely be distracting in long-lived source; replace with a neutral explanation of why the guard exists (ideally referencing the issue).
| Using `@Cacheable(sync = true)` with Feign clients may cause recursive cache invocation and result in an `IllegalStateException`. | ||
|
|
||
| This happens due to the interaction between Feign proxies and Spring Cache synchronization. | ||
|
|
||
| As a workaround, avoid using `sync = true` or disable Feign caching (`spring.cloud.openfeign.cache.enabled=false`). |
There was a problem hiding this comment.
This new “Limitations” note about @Cacheable(sync = true) causing recursive invocation/IllegalStateException isn’t tied to the stated PR problem (duplicate cache error handler execution) and there’s no reference/link for the claim. Either link it to a tracked issue/reproducer or adjust the documentation to describe the actual limitation being addressed by this PR.
| Using `@Cacheable(sync = true)` with Feign clients may cause recursive cache invocation and result in an `IllegalStateException`. | |
| This happens due to the interaction between Feign proxies and Spring Cache synchronization. | |
| As a workaround, avoid using `sync = true` or disable Feign caching (`spring.cloud.openfeign.cache.enabled=false`). | |
| When using Spring's cache abstraction with Feign clients, cache error handlers may be invoked more than once for a single Feign call. | |
| This behavior is due to the interaction between Feign proxies and Spring Cache infrastructure when handling cache failures. | |
| As a workaround, design cache error handlers to be idempotent and avoid side effects, or disable Feign caching (`spring.cloud.openfeign.cache.enabled=false`) if this behavior is problematic. |
| === FeignClientBuilder | ||
|
|
||
| `FeignClientBuilder` allows programmatic creation of Feign clients without using the `@FeignClient` annotation. | ||
|
|
||
| It builds clients in the same way as `@FeignClient`, but provides flexibility for dynamic use cases. | ||
|
|
||
| Unlike `@FeignClient`, which defines clients statically, `FeignClientBuilder` allows creating clients dynamically at runtime. | ||
|
|
||
| ==== Basic Usage |
There was a problem hiding this comment.
The added FeignClientBuilder section is unrelated to the caching bug described in the PR and significantly expands the scope of this change. If this is intentional, it should be split into a separate PR; otherwise please drop it from this one to keep the fix focused.
This PR fixes an issue where cache error handling is executed twice when using @Cacheable on Feign clients.
Problem
Caching was being applied twice:
This caused duplicate execution of cache error handling methods.
Solution
Added a ThreadLocal guard to prevent nested cache invocation inside FeignCachingInvocationHandlerFactory.
Result
Closes #681