From 045df81f14685969f6d9994594092f2aae698456 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 8 Jun 2023 17:27:09 +0200 Subject: [PATCH] Reduce ProxyCallbackFilter to key-only role after class generation Avoids memory leaks from ProxyCallbackFilter-contained Advisors. Includes consistent ProxyCallbackFilter#equals/hashCode methods. Closes gh-26266 Closes gh-30615 --- .../aop/framework/AdvisedSupport.java | 87 +++++++++++++++++-- .../aop/framework/CglibAopProxy.java | 87 +++++-------------- .../aop/framework/CglibProxyTests.java | 37 ++++++++ 3 files changed, 140 insertions(+), 71 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java b/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java index 90d5afef82d9..d88c58aa3bb3 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/AdvisedSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -32,6 +33,8 @@ import org.springframework.aop.DynamicIntroductionAdvice; import org.springframework.aop.IntroductionAdvisor; import org.springframework.aop.IntroductionInfo; +import org.springframework.aop.Pointcut; +import org.springframework.aop.PointcutAdvisor; import org.springframework.aop.TargetSource; import org.springframework.aop.support.DefaultIntroductionAdvisor; import org.springframework.aop.support.DefaultPointcutAdvisor; @@ -41,6 +44,7 @@ import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; +import org.springframework.util.ObjectUtils; /** * Base class for AOP proxy configuration managers. @@ -72,15 +76,13 @@ public class AdvisedSupport extends ProxyConfig implements Advised { /** Package-protected to allow direct access for efficiency. */ - @SuppressWarnings("serial") TargetSource targetSource = EMPTY_TARGET_SOURCE; /** Whether the Advisors are already filtered for the specific target class. */ private boolean preFiltered = false; /** The AdvisorChainFactory to use. */ - @SuppressWarnings("serial") - AdvisorChainFactory advisorChainFactory = new DefaultAdvisorChainFactory(); + private AdvisorChainFactory advisorChainFactory; /** Cache with Method as key and advisor chain List as value. */ private transient Map> methodCache; @@ -89,21 +91,22 @@ public class AdvisedSupport extends ProxyConfig implements Advised { * Interfaces to be implemented by the proxy. Held in List to keep the order * of registration, to create JDK proxy with specified order of interfaces. */ - @SuppressWarnings("serial") private List> interfaces = new ArrayList<>(); /** * List of Advisors. If an Advice is added, it will be wrapped * in an Advisor before being added to this List. */ - @SuppressWarnings("serial") private List advisors = new ArrayList<>(); + private List advisorKey = this.advisors; + /** * No-arg constructor for use as a JavaBean. */ public AdvisedSupport() { + this.advisorChainFactory = DefaultAdvisorChainFactory.INSTANCE; this.methodCache = new ConcurrentHashMap<>(32); } @@ -116,6 +119,15 @@ public AdvisedSupport(Class... interfaces) { setInterfaces(interfaces); } + /** + * Internal constructor for {@link #getConfigurationOnlyCopy()}. + * @since 6.0.10 + */ + private AdvisedSupport(AdvisorChainFactory advisorChainFactory, Map> methodCache) { + this.advisorChainFactory = advisorChainFactory; + this.methodCache = methodCache; + } + /** * Set the given object as target. @@ -520,15 +532,27 @@ protected void copyConfigurationFrom(AdvisedSupport other, TargetSource targetSo * replacing the TargetSource. */ AdvisedSupport getConfigurationOnlyCopy() { - AdvisedSupport copy = new AdvisedSupport(); + AdvisedSupport copy = new AdvisedSupport(this.advisorChainFactory, this.methodCache); copy.copyFrom(this); copy.targetSource = EmptyTargetSource.forClass(getTargetClass(), getTargetSource().isStatic()); - copy.advisorChainFactory = this.advisorChainFactory; copy.interfaces = new ArrayList<>(this.interfaces); copy.advisors = new ArrayList<>(this.advisors); + copy.advisorKey = new ArrayList<>(this.advisors.size()); + for (Advisor advisor : this.advisors) { + copy.advisorKey.add(new AdvisorKeyEntry(advisor)); + } return copy; } + void reduceToAdvisorKey() { + this.advisors = this.advisorKey; + this.methodCache = Collections.emptyMap(); + } + + Object getAdvisorKey() { + return this.advisorKey; + } + //--------------------------------------------------------------------- // Serialization support @@ -604,4 +628,51 @@ public int compareTo(MethodCacheKey other) { } } + + /** + * Stub for an Advisor instance that is just needed for key purposes, + * allowing for efficient equals and hashCode comparisons against the + * advice class and the pointcut. + * @since 6.0.10 + * @see #getConfigurationOnlyCopy() + * @see #getAdvisorKey() + */ + private static class AdvisorKeyEntry implements Advisor { + + private final Class adviceType; + + @Nullable + private String classFilterKey; + + @Nullable + private String methodMatcherKey; + + public AdvisorKeyEntry(Advisor advisor) { + this.adviceType = advisor.getAdvice().getClass(); + if (advisor instanceof PointcutAdvisor pointcutAdvisor) { + Pointcut pointcut = pointcutAdvisor.getPointcut(); + this.classFilterKey = ObjectUtils.identityToString(pointcut.getClassFilter()); + this.methodMatcherKey = ObjectUtils.identityToString(pointcut.getMethodMatcher()); + } + } + + @Override + public Advice getAdvice() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean equals(Object other) { + return (this == other || (other instanceof AdvisorKeyEntry otherEntry && + this.adviceType == otherEntry.adviceType && + ObjectUtils.nullSafeEquals(this.classFilterKey, otherEntry.classFilterKey) && + ObjectUtils.nullSafeEquals(this.methodMatcherKey, otherEntry.methodMatcherKey))); + } + + @Override + public int hashCode() { + return this.adviceType.hashCode(); + } + } + } diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java index a95c6c7ff2a1..cb7d8e7a0f26 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java @@ -26,14 +26,11 @@ import java.util.Set; import java.util.WeakHashMap; -import org.aopalliance.aop.Advice; import org.aopalliance.intercept.MethodInvocation; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.aop.Advisor; import org.springframework.aop.AopInvocationException; -import org.springframework.aop.PointcutAdvisor; import org.springframework.aop.RawTargetAccess; import org.springframework.aop.TargetSource; import org.springframework.aop.support.AopUtils; @@ -205,12 +202,21 @@ private Object buildProxy(@Nullable ClassLoader classLoader, boolean classOnly) types[x] = callbacks[x].getClass(); } // fixedInterceptorMap only populated at this point, after getCallbacks call above - enhancer.setCallbackFilter(new ProxyCallbackFilter( - this.advised.getConfigurationOnlyCopy(), this.fixedInterceptorMap, this.fixedInterceptorOffset)); + ProxyCallbackFilter filter = new ProxyCallbackFilter( + this.advised.getConfigurationOnlyCopy(), this.fixedInterceptorMap, this.fixedInterceptorOffset); + enhancer.setCallbackFilter(filter); enhancer.setCallbackTypes(types); // Generate the proxy class and create a proxy instance. - return (classOnly ? createProxyClass(enhancer) : createProxyClassAndInstance(enhancer, callbacks)); + // ProxyCallbackFilter has method introspection capability with Advisor access. + try { + return (classOnly ? createProxyClass(enhancer) : createProxyClassAndInstance(enhancer, callbacks)); + } + finally { + // Reduce ProxyCallbackFilter to key-only state for its class cache role + // in the CGLIB$CALLBACK_FILTER field, not leaking any Advisor state... + filter.advised.reduceToAdvisorKey(); + } } catch (CodeGenerationException | IllegalArgumentException ex) { throw new AopConfigException("Could not generate CGLIB subclass of " + this.advised.getTargetClass() + @@ -294,9 +300,9 @@ else if (logger.isDebugEnabled() && !Modifier.isPublic(mod) && !Modifier.isProte private Callback[] getCallbacks(Class rootClass) throws Exception { // Parameters used for optimization choices... - boolean exposeProxy = this.advised.isExposeProxy(); - boolean isFrozen = this.advised.isFrozen(); boolean isStatic = this.advised.getTargetSource().isStatic(); + boolean isFrozen = this.advised.isFrozen(); + boolean exposeProxy = this.advised.isExposeProxy(); // Choose an "aop" interceptor (used for AOP calls). Callback aopInterceptor = new DynamicAdvisedInterceptor(this.advised); @@ -776,7 +782,7 @@ public Object proceed() throws Throwable { */ private static class ProxyCallbackFilter implements CallbackFilter { - private final AdvisedSupport advised; + final AdvisedSupport advised; private final Map fixedInterceptorMap; @@ -857,9 +863,9 @@ public int accept(Method method) { // Proxy is not yet available, but that shouldn't matter. List chain = this.advised.getInterceptorsAndDynamicInterceptionAdvice(method, targetClass); boolean haveAdvice = !chain.isEmpty(); - boolean exposeProxy = this.advised.isExposeProxy(); boolean isStatic = this.advised.getTargetSource().isStatic(); boolean isFrozen = this.advised.isFrozen(); + boolean exposeProxy = this.advised.isExposeProxy(); if (haveAdvice || !isFrozen) { // If exposing the proxy, then AOP_PROXY must be used. if (exposeProxy) { @@ -921,63 +927,18 @@ public boolean equals(@Nullable Object other) { return false; } AdvisedSupport otherAdvised = otherCallbackFilter.advised; - if (this.advised.isFrozen() != otherAdvised.isFrozen()) { - return false; - } - if (this.advised.isExposeProxy() != otherAdvised.isExposeProxy()) { - return false; - } - if (this.advised.getTargetSource().isStatic() != otherAdvised.getTargetSource().isStatic()) { - return false; - } - if (!AopProxyUtils.equalsProxiedInterfaces(this.advised, otherAdvised)) { - return false; - } - // Advice instance identity is unimportant to the proxy class: - // All that matters is type and ordering. - if (this.advised.getAdvisorCount() != otherAdvised.getAdvisorCount()) { - return false; - } - Advisor[] thisAdvisors = this.advised.getAdvisors(); - Advisor[] thatAdvisors = otherAdvised.getAdvisors(); - for (int i = 0; i < thisAdvisors.length; i++) { - Advisor thisAdvisor = thisAdvisors[i]; - Advisor thatAdvisor = thatAdvisors[i]; - if (!equalsAdviceClasses(thisAdvisor, thatAdvisor)) { - return false; - } - if (!equalsPointcuts(thisAdvisor, thatAdvisor)) { - return false; - } - } - return true; - } - - private static boolean equalsAdviceClasses(Advisor a, Advisor b) { - return (a.getAdvice().getClass() == b.getAdvice().getClass()); - } - - private static boolean equalsPointcuts(Advisor a, Advisor b) { - // If only one of the advisor (but not both) is PointcutAdvisor, then it is a mismatch. - // Takes care of the situations where an IntroductionAdvisor is used (see SPR-3959). - return (!(a instanceof PointcutAdvisor pointcutAdvisor1) || - (b instanceof PointcutAdvisor pointcutAdvisor2 && - ObjectUtils.nullSafeEquals(pointcutAdvisor1.getPointcut(), pointcutAdvisor2.getPointcut()))); + return (this.advised.getAdvisorKey().equals(otherAdvised.getAdvisorKey()) && + AopProxyUtils.equalsProxiedInterfaces(this.advised, otherAdvised) && + ObjectUtils.nullSafeEquals(this.advised.getTargetClass(), otherAdvised.getTargetClass()) && + this.advised.getTargetSource().isStatic() == otherAdvised.getTargetSource().isStatic() && + this.advised.isFrozen() == otherAdvised.isFrozen() && + this.advised.isExposeProxy() == otherAdvised.isExposeProxy() && + this.advised.isOpaque() == otherAdvised.isOpaque()); } @Override public int hashCode() { - int hashCode = 0; - Advisor[] advisors = this.advised.getAdvisors(); - for (Advisor advisor : advisors) { - Advice advice = advisor.getAdvice(); - hashCode = 13 * hashCode + advice.getClass().hashCode(); - } - hashCode = 13 * hashCode + (this.advised.isFrozen() ? 1 : 0); - hashCode = 13 * hashCode + (this.advised.isExposeProxy() ? 1 : 0); - hashCode = 13 * hashCode + (this.advised.isOptimize() ? 1 : 0); - hashCode = 13 * hashCode + (this.advised.isOpaque() ? 1 : 0); - return hashCode; + return this.advised.getAdvisorKey().hashCode(); } } diff --git a/spring-context/src/test/java/org/springframework/aop/framework/CglibProxyTests.java b/spring-context/src/test/java/org/springframework/aop/framework/CglibProxyTests.java index 047974b454ba..3fddbf2af9fa 100644 --- a/spring-context/src/test/java/org/springframework/aop/framework/CglibProxyTests.java +++ b/spring-context/src/test/java/org/springframework/aop/framework/CglibProxyTests.java @@ -28,6 +28,7 @@ import org.springframework.aop.Pointcut; import org.springframework.aop.support.AopUtils; import org.springframework.aop.support.DefaultPointcutAdvisor; +import org.springframework.aop.support.annotation.AnnotationMatchingPointcut; import org.springframework.aop.testfixture.advice.CountingBeforeAdvice; import org.springframework.aop.testfixture.interceptor.NopInterceptor; import org.springframework.beans.testfixture.beans.ITestBean; @@ -35,6 +36,7 @@ import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextException; import org.springframework.context.support.ClassPathXmlApplicationContext; +import org.springframework.lang.NonNull; import org.springframework.lang.Nullable; import static org.assertj.core.api.Assertions.assertThat; @@ -304,6 +306,8 @@ public void testProxyAProxyWithAdditionalInterface() { CglibAopProxy cglib = new CglibAopProxy(as); ITestBean proxy1 = (ITestBean) cglib.getProxy(); + ITestBean proxy1a = (ITestBean) cglib.getProxy(); + assertThat(proxy1a.getClass()).isSameAs(proxy1.getClass()); mockTargetSource.setTarget(proxy1); as = new AdvisedSupport(new Class[]{}); @@ -313,6 +317,39 @@ public void testProxyAProxyWithAdditionalInterface() { ITestBean proxy2 = (ITestBean) cglib.getProxy(); assertThat(proxy2).isInstanceOf(Serializable.class); + assertThat(proxy2.getClass()).isNotSameAs(proxy1.getClass()); + + ITestBean proxy2a = (ITestBean) cglib.getProxy(); + assertThat(proxy2a).isInstanceOf(Serializable.class); + assertThat(proxy2a.getClass()).isSameAs(proxy2.getClass()); + + mockTargetSource.setTarget(proxy1); + as = new AdvisedSupport(new Class[]{}); + as.setTargetSource(mockTargetSource); + as.addAdvisor(new DefaultPointcutAdvisor(new AnnotationMatchingPointcut(Nullable.class), new NopInterceptor())); + cglib = new CglibAopProxy(as); + + ITestBean proxy3 = (ITestBean) cglib.getProxy(); + assertThat(proxy3).isInstanceOf(Serializable.class); + assertThat(proxy3.getClass()).isNotSameAs(proxy2.getClass()); + + ITestBean proxy3a = (ITestBean) cglib.getProxy(); + assertThat(proxy3a).isInstanceOf(Serializable.class); + assertThat(proxy3a.getClass()).isSameAs(proxy3.getClass()); + + mockTargetSource.setTarget(proxy1); + as = new AdvisedSupport(new Class[]{}); + as.setTargetSource(mockTargetSource); + as.addAdvisor(new DefaultPointcutAdvisor(new AnnotationMatchingPointcut(NonNull.class), new NopInterceptor())); + cglib = new CglibAopProxy(as); + + ITestBean proxy4 = (ITestBean) cglib.getProxy(); + assertThat(proxy4).isInstanceOf(Serializable.class); + assertThat(proxy4.getClass()).isNotSameAs(proxy3.getClass()); + + ITestBean proxy4a = (ITestBean) cglib.getProxy(); + assertThat(proxy4a).isInstanceOf(Serializable.class); + assertThat(proxy4a.getClass()).isSameAs(proxy4.getClass()); } @Test