-
Notifications
You must be signed in to change notification settings - Fork 828
fix: prevent duplicate cache error handler execution in FeignCachingInvocationHandlerFactory #1363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -745,6 +745,14 @@ public interface DemoClient { | |
|
|
||
| You can also disable the feature via property `spring.cloud.openfeign.cache.enabled=false`. | ||
|
|
||
| ==== Limitations | ||
|
|
||
| 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`). | ||
|
|
||
|
|
||
| [[spring-requestmapping-support]] | ||
| === Spring @RequestMapping Support | ||
|
|
@@ -879,6 +887,46 @@ protected interface DemoFeignClient { | |
| } | ||
| ---- | ||
|
|
||
|
|
||
| === 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 | ||
|
Comment on lines
+891
to
+899
|
||
|
|
||
| [source,java,indent=0] | ||
| ---- | ||
| @Autowired | ||
| private ApplicationContext applicationContext; | ||
|
|
||
| FeignClientBuilder builder = new FeignClientBuilder(applicationContext); | ||
|
|
||
| MyClient client = builder | ||
| .forType(MyClient.class, "myClient") | ||
| .url("http://localhost:8080") | ||
| .build(); | ||
| ---- | ||
|
|
||
| ==== Configuration Options | ||
|
|
||
| * `url(String url)` - Sets the target URL | ||
| * `path(String path)` - Adds a base path | ||
| * `contextId(String contextId)` - Unique identifier | ||
| * `dismiss404(boolean)` - Ignore 404 errors | ||
| * `inheritParentContext(boolean)` - Inherit parent config | ||
| * `fallback(Class<? extends T>)` - Fallback class | ||
| * `customize(FeignBuilderCustomizer)` - Customize Feign builder | ||
|
|
||
| ==== When to Use | ||
|
|
||
| * Dynamic client creation | ||
| * Runtime configuration | ||
| * When `@FeignClient` is not sufficient | ||
|
|
||
| [[reactive-support]] | ||
| === Reactive Support | ||
| As at the time of active development of Spring Cloud OpenFeign, the https://github.com/OpenFeign/feign[OpenFeign project] did not support reactive clients, such as https://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/web/reactive/function/client/WebClient.html[Spring WebClient], such support could not be added to Spring Cloud OpenFeign either. | ||
|
|
@@ -893,6 +941,7 @@ We discourage using Feign clients in the early stages of application lifecycle, | |
| Similarly, depending on how you are using your Feign clients, you may see initialization errors when starting your application. To work around this problem you can use an `ObjectProvider` when autowiring your client. | ||
|
|
||
| [source,java,indent=0] | ||
|
|
||
| ---- | ||
| @Autowired | ||
| ObjectProvider<TestFeignClient> testFeignClient; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,9 @@ public class FeignCachingInvocationHandlerFactory implements InvocationHandlerFa | |
|
|
||
| private final CacheInterceptor cacheInterceptor; | ||
|
|
||
| // ADDED: ThreadLocal guard | ||
| private static final ThreadLocal<Boolean> CACHE_IN_PROGRESS = ThreadLocal.withInitial(() -> false); | ||
|
|
||
| public FeignCachingInvocationHandlerFactory(InvocationHandlerFactory delegateFactory, | ||
| CacheInterceptor cacheInterceptor) { | ||
| this.delegateFactory = delegateFactory; | ||
|
|
@@ -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()) { | ||
|
Comment on lines
42
to
+58
|
||
| return delegateHandler.invoke(proxy, method, args); | ||
| } | ||
|
|
||
| try { | ||
| CACHE_IN_PROGRESS.set(true); | ||
|
|
||
| return cacheInterceptor.invoke(new MethodInvocation() { | ||
|
Comment on lines
+57
to
+65
|
||
| @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; | ||
| } | ||
| }); | ||
| } | ||
| finally { | ||
| CACHE_IN_PROGRESS.remove(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new “Limitations” note about
@Cacheable(sync = true)causing recursive invocation/IllegalStateExceptionisn’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.