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 an UndeclaredThrowableException
, to match the behaviour of java.lang.reflect.Proxy
, so that callers would see the target's exception wrapped in an UndeclaredThrowableException
. But starting with 5.2.RC2, the UndeclaredThrowableException
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 an UndeclaredThrowableException
, 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.
@Test
void givenCglibProxyAndNoRollbackForRuntimeException_whenUndeclaredCheckedException_thenRollsBack() {
assertThat(Enhancer.isEnhanced(myClass.getClass())).isTrue();
assertThrowsUndeclaredThrowableException(() -> myClass.throwException(new CheckedException()));
verify(platformTransactionManager).rollback(any()); // fails in 5.2.RC1, passes in 5.2.RC2
}
@Test
void givenJdkProxyAndNoRollbackForRuntimeException_whenUndeclaredCheckedException_thenRollsBack() {
assertThat(Proxy.isProxyClass(myInterfaceImpl.getClass())).isTrue();
assertThrowsUndeclaredThrowableException(() -> myInterfaceImpl.throwException(new CheckedException()));
verify(platformTransactionManager, never()).rollback(any());
}
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:
* Fix CGLIB
* Revert the error-handling to use CGLIB's UndeclaredThrowableTransformer
* Add a test suite that can catch regressions like the one in 5.2.RC2
Comment From: LeMikaelF
I pushed a commit that fixes the checkStyle issues.
For reference, I found this other related issue: https://github.com/spring-projects/spring-framework/issues/26854