-
Notifications
You must be signed in to change notification settings - Fork 38.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consistent handling of undeclared checked exceptions in CGLIB proxies #32469
Consistent handling of undeclared checked exceptions in CGLIB proxies #32469
Conversation
4cdf488
to
a159dd1
Compare
I pushed a commit that fixes the checkStyle issues. For reference, I found this other related issue: #26854 |
} | ||
private final boolean isConstructor = Constants.CONSTRUCTOR_NAME.equals(sig.getName()); | ||
private Block handler = begin_block(); | ||
private boolean calToSuperWasSeen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo perhaps in cal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm preparing a revision of this to separate ClassLoaderAwareGeneratorStrategy
from an UndeclaredThrowableStrategy
delegate, to be committed soon. Noticed this one already, renaming it to callToSuperSeen
.
The commit skips using UndeclaredThrowableStrategy for Kotlin classes in CglibAopProxy in order to fix a related regression caused by spring-projectsgh-32469. See spring-projectsgh-33585
This PR fixes a regression in 5.2.RC2.
Before this version, if an object was proxied with CGLIB and threw an undeclared checked exception, for example using lombok's
@SneakyThrows
, the proxy interceptors would see the target's exception as is, and the CGLIB generated class would then wrap the exception in anUndeclaredThrowableException
, to match the behaviour ofjava.lang.reflect.Proxy
, so that callers would see the target's exception wrapped in anUndeclaredThrowableException
. But starting with 5.2.RC2, theUndeclaredThrowableException
is created in the process of invoking the target, inside the proxy, and so the interceptors see the target's exception only after it has been wrapped in anUndeclaredThrowableException
, instead of seeing the raw exception from the target. An exception was since then added to prevent this in Kotlin applications, where all exceptions are unchecked (relevant issue), but not for Java.Concretely, this means for example that the rollback behaviour of
@Transactional
methods changes in 5.2.RC2 for Java applications. One situation where this can occur is that if an application uses CGLIB proxies and undeclared checked exceptions, transactions will stop rolling back on those exceptions with the 5.2 upgrade. Another case is that if a post-5.2 application uses a CGLIB proxy with@Transactional
, extracting an interface from a bean can cause the transaction to stop rolling back on undeclared checked exceptions. Below is the relevant section from a reproducer.This regression also seems to affect Resilience4j's circuit breaker, according to one report.
The intent behind the new error-handling introduced in 5.2.RC2 was to avoid a bug in CGLIB, but it also changed the behaviour of AOP interceptors. I tried fixing this by reusing a similar error-handling pattern, but the issue is that CGLIB proxies don't always use interceptors, in some cases they use dispatchers to directly invoke the target, so the user-facing error behaviour became really messy.
The best solution to this issue is therefore to do the undeclared-exception handling in the generated code. It turns out, there's a fork of CGLIB (also Apache 2) where the issue that prompted the custom error-handling in the first place was fixed (issue and unmerged PR in CGLIB). I reused the implementation from that PR committed it with the original author's name (@hengyunabc), and did some light refactor to it.
This MR therefore consists of 3 parts:
UndeclaredThrowableTransformer