Improved checks on final methods and cglib proxy creation; fixes gh-1569

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 <T> boolean anyFinalMethods(T object, Class<T> iface) {
+	private static <T> 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;
 				}
 			}

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();
+		}
+
+	}
+
 }

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;