AdvisedSupport.MethodCacheKey should check for logical equality as well as identity
#33915
Assignees
Labels
in: core
Issues in core modules (aop, beans, core, context, expression)
type: enhancement
A general enhancement
Milestone
Comments
spring-projects-issues
added
the
status: waiting-for-triage
bclozel
added
the
in: core
|
@bclozel Hello, the code and README in this repository provide a more detailed description of the issue. I would be very grateful if you could help confirm this problem. |
sbrannen
changed the title
AdvisedSupport.MethodCacheKey#equals, using == to determine object equality may lead to issues
sbrannen
changed the title
AdvisedSupport.MethodCacheKey#equals, using == to determine object equality may lead to issuesAdvisedSupport.MethodCacheKey#equals should check for equality as well as identity
sbrannen
added a commit
that referenced
this issue
Dec 1, 2024
This commit introduces a test which verifies how many times the matches() method of a StaticMethodMatcherPointcut is invoked during ApplicationContext startup as well as during actual method invocations via the advice chain, which also indirectly tests the behavior of the equals() implementation in AdvisedSupport.MethodCacheKey. In addition, this commit revises BeanFactoryTransactionTests to assert that a transaction is started for the setAge() method. See gh-33915
sbrannen
added a commit
to sbrannen/spring-framework
that referenced
this issue
Dec 1, 2024
Prior to this commit, the equals() implementation in AdvisedSupport's MethodCacheKey only considered methods to be equal based on an identity comparison (`==`). However, an identity comparison is not always sufficient due to the following. - Class#getDeclaredMethods() always returns "child copies" of the underlying Method instances -- which means that `equals()` should be used instead of (or in addition to) `==` whenever the compared Method instances can come from different sources. With this commit, the equals() implementation in MethodCacheKey now considers methods equal based on identity or logical equality, giving preference to the quicker identity check. See spring-projectsgh-32586 Closes spring-projectsgh-33915
sbrannen
changed the title
AdvisedSupport.MethodCacheKey#equals should check for equality as well as identityAdvisedSupport.MethodCacheKey should check for logical equality as well as identity
sbrannen
added
type: enhancement
|
Congratulations on submitting your first issue for the Spring Framework! 👍 And thanks for the detailed explanation and sample project. In commit 320831b, I introduced In addition, I pushed a proposed enhancement for main...sbrannen:spring-framework:issues/gh-33915-AdvisedSupport-MethodCacheKey As a side note, this is also related to: Regards, Sam |
sbrannen
added a commit
to sbrannen/spring-framework
that referenced
this issue
Dec 2, 2024
Prior to this commit, the equals() implementation in AdvisedSupport's MethodCacheKey only considered methods to be equal based on an identity comparison (`==`). However, an identity comparison is not always sufficient due to the following. - Class#getDeclaredMethods() always returns "child copies" of the underlying Method instances -- which means that `equals()` should be used instead of (or in addition to) `==` whenever the compared Method instances can come from different sources. With this commit, the equals() implementation in MethodCacheKey now considers methods equal based on identity or logical equality, giving preference to the quicker identity check. See spring-projectsgh-32586 Closes spring-projectsgh-33915
sbrannen
added a commit
that referenced
this issue
Dec 4, 2024
Prior to this commit, the equals() implementation in AdvisedSupport's MethodCacheKey only considered methods to be equal based on an identity comparison (`==`), which led to duplicate entries in the method cache for the same logical method. This is caused by the fact that AdvisedSupport's getInterceptorsAndDynamicInterceptionAdvice() method is invoked at various stages with different Method instances for the same method: 1) when creating the proxy 2) when invoking the method via the proxy The reason the Method instances are different is due to the following. - Methods such as Class#getDeclaredMethods() and Class#getDeclaredMethod() always returns "child copies" of the underlying Method instances -- which means that `equals()` should be used instead of (or in addition to) `==` whenever the compared Method instances can come from different sources. With this commit, the equals() implementation in MethodCacheKey now considers methods equal based on identity or logical equality, giving preference to the quicker identity check. See gh-32586 Closes gh-33915
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In the production environment, I discovered that
AdvisedSupport#methodCachecontains two "identical" keys, both corresponding to the same method object.Experimentally, I found that the
methodCacheestablished during bean initialization is practically useless. During actual method invocation, the interceptor chain retrieved viamethodCache.get(cacheKey)turns out to be empty. Thus, the interceptor chain is fetched again usingadvisorChainFactoryand then placed into themethodCache. This means that the initialization logic related togetInterceptorsAndDynamicInterceptionAdvicewill not only execute during bean initialization but also during the first method invocation.Returning to the issue itself, since each call to
getClass().getDeclaredMethods()[0]results in aMethodobject that, when compared using==, returnsfalse(even if it's the same method), I believe theequalsmethod ofMethodCacheKeyis poorly designed.The text was updated successfully, but these errors were encountered: