Fix CreateAopProxyInterceptor in the Spring core-patch indeed changes the implementation of the Spring AOP proxy#739
Merged
wu-sheng merged 2 commits intoapache:mainfrom Dec 15, 2024
Conversation
…spring aop proxy implement
youjie23
changed the title
3 tasks
Member
|
You missed the update of changes.md. Please fix it if all tests can pass. |
wu-sheng
reviewed
Dec 15, 2024
| private boolean onlyImplementsEnhancedInstanceAndSpringProxy(AdvisedSupport advisedSupport) { | ||
| Class<?>[] ifcs = advisedSupport.getProxiedInterfaces(); | ||
| Class targetClass = advisedSupport.getTargetClass(); | ||
| return ifcs.length == 2 && EnhancedInstance.class.isAssignableFrom(targetClass) && SpringProxy.class.isAssignableFrom(targetClass); |
Member
There was a problem hiding this comment.
When is SpringProxy.class added into the hierarchy tree?
Contributor
Author
There was a problem hiding this comment.
Thank you for the review.
As shown below, the hasNoUserSuppliedProxyInterfaces method in DefaultAopProxyFactory is defined as follows:
spring4/spring5
/**
* Determine whether the supplied {@link AdvisedSupport} has only the
* {@link org.springframework.aop.SpringProxy} interface specified
* (or no proxy interfaces specified at all).
*/
private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
Class<?>[] ifcs = config.getProxiedInterfaces();
return (ifcs.length == 0 || (ifcs.length == 1 && SpringProxy.class.isAssignableFrom(ifcs[0])));
}
spring3
/**
* Determine whether the supplied {@link AdvisedSupport} has only the
* {@link org.springframework.aop.SpringProxy} interface specified
* (or no proxy interfaces specified at all).
*/
private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
Class[] interfaces = config.getProxiedInterfaces();
return (interfaces.length == 0 || (interfaces.length == 1 && SpringProxy.class.equals(interfaces[0])));
}
Member
There was a problem hiding this comment.
OK, this seems ok. Could you update the changelog? Then I could merge this PR.
Contributor
Author
There was a problem hiding this comment.
Thank you for the review.
The Changes.md file has been updated.
…spring aop proxy implement
wu-sheng
changed the title
| public void testInterceptClassImplementsEnhancedInstance() throws Throwable { | ||
| doReturn(MockClassImplementsEnhancedInstance.class).when(advisedSupport).getTargetClass(); | ||
| doReturn(MockClassImplementsEnhancedInstance.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces(); | ||
| assertThat(true, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false))); |
Contributor
There was a problem hiding this comment.
According to the answer in this SO thread, we basically should not expect classes that both use @Transactional annotation and being enhanced by SkyWalking Agent to use JDK Proxy.
We are facing startup failures after upgrading to SkyWalking 9.4.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix apache/skywalking#12858 The
CreateAopProxyInterceptorin the Spring core-patch indeed changes the implementation of the Spring AOP proxyThe current version of
CreateAopProxyInterceptordoes not account for scenarios where the proxy target class implementsUserSuppliedInterfaceand should work correctly with onlyJdkDynamicAopProxy. This can lead to inconsistencies in behavior when the SkyWalking agent is used.This PR fixes the issue by adding restrictions on when the behavior changes, ensuring that the proxy mechanism reverts to using
JdkDynamicAopProxywhen appropriate. Specifically, it ensures that the application's behavior remains consistent both with and without the SkyWalking agent enhancement.