From 83a73d0cc8da5e489262f6402b5c0c4b87b0f8cc Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Mon, 7 Jun 2021 13:34:33 -0400 Subject: [PATCH] GH-244: Invoke Proxy for @Recover Method Resolves https://github.com/spring-projects/spring-retry/issues/244 The `RecoverAnnotationRecoveryHandler` only has access to the undecorated raw bean. Use the `RetryContext` to make the proxy (if CGLIB) available to the recoverer. * Move cleanup to a finally block. * Support JDK proxies too. * Add CGLib proxy test; JDK proxies are covered by existing tests. --- .../RecoverAnnotationRecoveryHandler.java | 33 +++++++++- .../RetryOperationsInterceptor.java | 33 +++++++--- .../retry/annotation/EnableRetryTests.java | 63 +++++++++++++++++++ 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java b/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java index f075201..c0511e4 100644 --- a/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java +++ b/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java @@ -25,7 +25,9 @@ import java.util.Map; import org.springframework.classify.SubclassClassifier; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.retry.ExhaustedRetryException; +import org.springframework.retry.RetryContext; import org.springframework.retry.interceptor.MethodInvocationRecoverer; +import org.springframework.retry.support.RetrySynchronizationManager; import org.springframework.util.ReflectionUtils; import org.springframework.util.ReflectionUtils.MethodCallback; import org.springframework.util.StringUtils; @@ -76,8 +78,25 @@ public class RecoverAnnotationRecoveryHandler implements MethodInvocationReco boolean methodAccessible = method.isAccessible(); try { ReflectionUtils.makeAccessible(method); + RetryContext context = RetrySynchronizationManager.getContext(); + Object proxy = null; + if (context != null) { + proxy = context.getAttribute("___proxy___"); + if (proxy != null) { + Method proxyMethod = findMethodOnProxy(method, proxy); + if (proxyMethod == null) { + proxy = null; + } + else { + method = proxyMethod; + } + } + } + if (proxy == null) { + proxy = this.target; + } @SuppressWarnings("unchecked") - T result = (T) ReflectionUtils.invokeMethod(method, this.target, argsToUse); + T result = (T) ReflectionUtils.invokeMethod(method, proxy, argsToUse); return result; } finally { @@ -87,6 +106,18 @@ public class RecoverAnnotationRecoveryHandler implements MethodInvocationReco } } + private Method findMethodOnProxy(Method method, Object proxy) { + try { + return proxy.getClass().getMethod(method.getName(), method.getParameterTypes()); + } + catch (NoSuchMethodException e) { + return null; + } + catch (SecurityException e) { + return null; + } + } + private Method findClosestMatch(Object[] args, Class cause) { Method result = null; diff --git a/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java index eb77b32..80771bd 100644 --- a/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/interceptor/RetryOperationsInterceptor.java @@ -23,6 +23,7 @@ import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryCallback; import org.springframework.retry.RetryContext; import org.springframework.retry.RetryOperations; +import org.springframework.retry.support.RetrySynchronizationManager; import org.springframework.retry.support.RetryTemplate; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -64,11 +65,12 @@ public class RetryOperationsInterceptor implements MethodInterceptor { this.recoverer = recoverer; } + @Override public Object invoke(final MethodInvocation invocation) throws Throwable { String name; - if (StringUtils.hasText(label)) { - name = label; + if (StringUtils.hasText(this.label)) { + name = this.label; } else { name = invocation.getMethod().toGenericString(); @@ -78,9 +80,10 @@ public class RetryOperationsInterceptor implements MethodInterceptor { RetryCallback retryCallback = new MethodInvocationRetryCallback( invocation, label) { + @Override public Object doWithRetry(RetryContext context) throws Exception { - context.setAttribute(RetryContext.NAME, label); + context.setAttribute(RetryContext.NAME, this.label); /* * If we don't copy the invocation carefully it won't keep a reference to @@ -88,9 +91,10 @@ public class RetryOperationsInterceptor implements MethodInterceptor { * specialise to ReflectiveMethodInvocation (but how often would another * implementation come along?). */ - if (invocation instanceof ProxyMethodInvocation) { + if (this.invocation instanceof ProxyMethodInvocation) { + context.setAttribute("___proxy___", ((ProxyMethodInvocation) this.invocation).getProxy()); try { - return ((ProxyMethodInvocation) invocation).invocableClone().proceed(); + return ((ProxyMethodInvocation) this.invocation).invocableClone().proceed(); } catch (Exception e) { throw e; @@ -111,9 +115,19 @@ public class RetryOperationsInterceptor implements MethodInterceptor { }; - if (recoverer != null) { - ItemRecovererCallback recoveryCallback = new ItemRecovererCallback(invocation.getArguments(), recoverer); - return this.retryOperations.execute(retryCallback, recoveryCallback); + if (this.recoverer != null) { + ItemRecovererCallback recoveryCallback = new ItemRecovererCallback(invocation.getArguments(), + this.recoverer); + try { + Object recovered = this.retryOperations.execute(retryCallback, recoveryCallback); + return recovered; + } + finally { + RetryContext context = RetrySynchronizationManager.getContext(); + if (context != null) { + context.removeAttribute("__proxy__"); + } + } } return this.retryOperations.execute(retryCallback); @@ -138,8 +152,9 @@ public class RetryOperationsInterceptor implements MethodInterceptor { this.recoverer = recoverer; } + @Override public Object recover(RetryContext context) { - return recoverer.recover(args, context.getLastThrowable()); + return this.recoverer.recover(this.args, context.getLastThrowable()); } } diff --git a/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java b/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java index d42da62..8e3610d 100644 --- a/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java +++ b/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java @@ -21,11 +21,14 @@ import java.util.Map; import java.util.Properties; import org.aopalliance.intercept.MethodInterceptor; +import org.aopalliance.intercept.MethodInvocation; import org.junit.Test; import org.springframework.aop.framework.Advised; import org.springframework.aop.support.AopUtils; +import org.springframework.beans.BeansException; import org.springframework.beans.DirectFieldAccessor; +import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -89,6 +92,9 @@ public class EnableRetryTests { TestProxyConfiguration.class); Service service = context.getBean(Service.class); assertTrue(AopUtils.isCglibProxy(service)); + RecoverableService recoverable = context.getBean(RecoverableService.class); + recoverable.service(); + assertTrue(recoverable.isOtherAdviceCalled()); context.close(); } @@ -270,6 +276,53 @@ public class EnableRetryTests { return new Service(); } + @Bean + public RecoverableService recoverable() { + return new RecoverableService(); + } + + @Bean + public static AdviceBPP bpp() { + return new AdviceBPP(); + } + + static class AdviceBPP implements BeanPostProcessor, Ordered { + + @Override + public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException { + + return bean; + } + + @Override + public Object postProcessAfterInitialization(final Object bean, String beanName) throws BeansException { + + if (bean instanceof RecoverableService) { + Advised advised = (Advised) bean; + advised.addAdvice(new MethodInterceptor() { + + @Override + public Object invoke(MethodInvocation invocation) throws Throwable { + + if (invocation.getMethod().getName().equals("recover")) { + ((RecoverableService) bean).setOtherAdviceCalled(); + } + return invocation.proceed(); + } + + }); + return bean; + } + return bean; + } + + @Override + public int getOrder() { + return Integer.MAX_VALUE; + } + + } + } @Configuration @@ -475,6 +528,8 @@ public class EnableRetryTests { private Throwable cause; + boolean otherAdviceCalled; + @Retryable(RuntimeException.class) public void service() { this.count++; @@ -494,6 +549,14 @@ public class EnableRetryTests { return this.count; } + public void setOtherAdviceCalled() { + this.otherAdviceCalled = true; + } + + public boolean isOtherAdviceCalled() { + return this.otherAdviceCalled; + } + } @Retryable(RuntimeException.class)