From aea67cb0bbf6c139c824c2ace9df0b7c03d2dc75 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 6 Aug 2020 17:13:45 +0200 Subject: [PATCH] Improved checks on final methods and cglib proxy creation; fixes gh-1569 --- .../async/ExecutorBeanPostProcessor.java | 21 +++-- .../async/ExecutorBeanPostProcessorTests.java | 78 +++++++++++++++++++ .../async/issues/issue410/Issue410Tests.java | 4 +- 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessor.java index c3671e2e2..51dbb9b42 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessor.java @@ -120,7 +120,7 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { private Object wrapExecutor(Object bean) { Executor executor = (Executor) bean; - boolean methodFinal = anyFinalMethods(executor, Executor.class); + boolean methodFinal = anyFinalMethods(executor); boolean classFinal = Modifier.isFinal(bean.getClass().getModifiers()); boolean cglibProxy = !methodFinal && !classFinal; try { @@ -144,7 +144,7 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { private Object wrapThreadPoolTaskExecutor(Object bean) { ThreadPoolTaskExecutor executor = (ThreadPoolTaskExecutor) bean; boolean classFinal = Modifier.isFinal(bean.getClass().getModifiers()); - boolean methodsFinal = anyFinalMethods(executor, ThreadPoolTaskExecutor.class); + boolean methodsFinal = anyFinalMethods(executor); boolean cglibProxy = !classFinal && !methodsFinal; return createThreadPoolTaskExecutorProxy(bean, cglibProxy, executor); } @@ -152,7 +152,7 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { private Object wrapExecutorService(Object bean) { ExecutorService executor = (ExecutorService) bean; boolean classFinal = Modifier.isFinal(bean.getClass().getModifiers()); - boolean methodFinal = anyFinalMethods(executor, ExecutorService.class); + boolean methodFinal = anyFinalMethods(executor); boolean cglibProxy = !classFinal && !methodFinal; return createExecutorServiceProxy(bean, cglibProxy, executor); } @@ -160,7 +160,7 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { private Object wrapScheduledExecutorService(Object bean) { ScheduledExecutorService executor = (ScheduledExecutorService) bean; boolean classFinal = Modifier.isFinal(bean.getClass().getModifiers()); - boolean methodFinal = anyFinalMethods(executor, ExecutorService.class); + boolean methodFinal = anyFinalMethods(executor); boolean cglibProxy = !classFinal && !methodFinal; return createScheduledExecutorServiceProxy(bean, cglibProxy, executor); } @@ -168,7 +168,7 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { private Object wrapAsyncTaskExecutor(Object bean) { AsyncTaskExecutor executor = (AsyncTaskExecutor) bean; boolean classFinal = Modifier.isFinal(bean.getClass().getModifiers()); - boolean methodsFinal = anyFinalMethods(executor, AsyncTaskExecutor.class); + boolean methodsFinal = anyFinalMethods(executor); boolean cglibProxy = !classFinal && !methodsFinal; return createAsyncTaskExecutorProxy(bean, cglibProxy, executor); } @@ -304,12 +304,17 @@ class ExecutorBeanPostProcessor implements BeanPostProcessor { return this.sleuthAsyncProperties; } - private static boolean anyFinalMethods(T object, Class iface) { + private static boolean anyFinalMethods(T object) { try { - for (Method method : ReflectionUtils.getDeclaredMethods(iface)) { + for (Method method : ReflectionUtils + .getAllDeclaredMethods(object.getClass())) { + if (method.getDeclaringClass().equals(Object.class)) { + continue; + } Method m = ReflectionUtils.findMethod(object.getClass(), method.getName(), method.getParameterTypes()); - if (m != null && Modifier.isFinal(m.getModifiers())) { + if (m != null && Modifier.isPublic(m.getModifiers()) + && Modifier.isFinal(m.getModifiers())) { return true; } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java index e75b8fe5e..8f856aa71 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java @@ -27,6 +27,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; @@ -34,6 +35,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import brave.Tracing; import org.aopalliance.aop.Advice; import org.assertj.core.api.BDDAssertions; +import org.awaitility.Awaitility; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -340,6 +342,21 @@ public class ExecutorBeanPostProcessorTests { then(wasCalled).isTrue(); } + @Test + public void should_use_cglib_proxy_when_an_executor_has_a_final_package_protected_method() { + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( + this.beanFactory); + ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(10); + ScheduledThreadPoolExecutor wrappedExecutor = (ScheduledThreadPoolExecutor) beanPostProcessor + .postProcessAfterInitialization(executor, "executor"); + + then(AopUtils.isCglibProxy(wrappedExecutor)).isTrue(); + + AtomicBoolean wasCalled = new AtomicBoolean(false); + wrappedExecutor.execute(() -> wasCalled.set(true)); + Awaitility.await().untilAsserted(() -> then(wasCalled).isTrue()); + } + @Test public void should_use_jdk_proxy_when_executor_service_has_final_methods() throws Exception { @@ -459,6 +476,44 @@ public class ExecutorBeanPostProcessorTests { } + // #1569 + @Test + public void should_use_jdk_proxy_when_executor_has_any_final_methods() { + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( + this.beanFactory); + + Executor wrappedExecutor = (Executor) beanPostProcessor + .postProcessAfterInitialization(new ExecutorWithFinalMethod(), + "executorWithFinalMethod"); + + then(AopUtils.isJdkDynamicProxy(wrappedExecutor)).isTrue(); + then(AopUtils.isCglibProxy(wrappedExecutor)).isFalse(); + AtomicBoolean wasCalled = new AtomicBoolean(false); + wrappedExecutor.execute(() -> { + wasCalled.set(true); + }); + then(wasCalled).isTrue(); + } + + // #1569 + @Test + public void should_use_jdk_proxy_when_executor_has_an_inherited_final_methods() { + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( + this.beanFactory); + + Executor wrappedExecutor = (Executor) beanPostProcessor + .postProcessAfterInitialization(new ExecutorWithInheritedFinalMethod(), + "executorWithFinalMethod"); + + then(AopUtils.isJdkDynamicProxy(wrappedExecutor)).isTrue(); + then(AopUtils.isCglibProxy(wrappedExecutor)).isFalse(); + AtomicBoolean wasCalled = new AtomicBoolean(false); + wrappedExecutor.execute(() -> { + wasCalled.set(true); + }); + then(wasCalled).isTrue(); + } + class Foo implements Executor { @Override @@ -504,4 +559,27 @@ public class ExecutorBeanPostProcessorTests { } + static class ExecutorWithFinalMethod implements Executor { + + @Override + public void execute(Runnable command) { + command.run(); + } + + public final void foo() { + + } + + } + + static class ExecutorWithInheritedFinalMethod extends ExecutorWithFinalMethod + implements Executor { + + @Override + public void execute(Runnable command) { + command.run(); + } + + } + } diff --git a/tests/spring-cloud-sleuth-instrumentation-async-tests/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java b/tests/spring-cloud-sleuth-instrumentation-async-tests/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java index d27298b58..0ed5a58aa 100644 --- a/tests/spring-cloud-sleuth-instrumentation-async-tests/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java +++ b/tests/spring-cloud-sleuth-instrumentation-async-tests/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java @@ -16,7 +16,6 @@ package org.springframework.cloud.sleuth.instrument.async.issues.issue410; -import java.lang.invoke.MethodHandles; import java.util.Date; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; @@ -66,8 +65,7 @@ import static org.assertj.core.api.BDDAssertions.then; properties = { "ribbon.eureka.enabled=false", "feign.hystrix.enabled=false" }) public class Issue410Tests { - private static final Log log = LogFactory - .getLog(MethodHandles.lookup().lookupClass()); + private static final Log log = LogFactory.getLog(Issue410Tests.class); @Autowired Environment environment;