AOP Alliance with Kotlin: Joinpoint.proceed() on void method leads to IllegalStateException

[eiselems]

Spring Boot Version used: 2.2.0

Steps to reproduce

1. Use kotlin
2. Implement the MethodInterceptor interface
3. Auto generate the methods - it generates override fun invoke(invocation: MethodInvocation?): Any
4. Call invocation.proceed on a method returning null

Analysis so far:

• Kotlin does not detect the nullability of the interface and the proceed() method correctly.

Suggestion:

• Add Spring's Nullable annotation since it is based on JSR-305 (javax.annotation) @Nullable to at least the interface. The method also does not get picked up by the compiler (it is indicating Any!) - but that doesn't seem to be an issue.

Fix:
Replace the return type of the implemented interface method with Any? (instead of Any).

Is this my bad or should the interface suggest that there could be a null value?

[sdeleuze]

There could indeed be null value returned, but I think we originally did not annotate those APIs with null safety annotation because they are part of Spring's variant of the AOP Alliance interfaces under the org.aopalliance package.

Any thoughts @jhoeller about adding or not null safety annotations to classes under the org.aopalliance package?

[jhoeller]

In general, we can actively maintain our AOP Alliance variant since it's effectively a fork anyway. At this point we only updated some javadoc IIRC.

Adding org.springframework.lang annotations would be rather circular there, so could we simply add the JSR-305 @Nullable annotation to that Joinpoint.proceed() declaration for the time being?

[jhoeller]

There seem to be some subtle implications with class loading here, with e.g. our ExposeInvocationInterceptorTests failing to load a non-public class that refers to the MethodInvocation type once we declare any nullability annotations on them (no matter whether our own or the JSR-305 variant). I'm not quite sure why yet but it is an indication that we might have to leave the AOP Alliance interfaces as-is there in terms of their classloading-relevant signature.

What's your use case for implementing MethodInvocation directly, actually? Seems to be a rather unusual thing to do. For us, this is mainly a common user-level abstraction of our ReflectiveMethodInvocation implementation, not an SPI interface to be implemented on its own.

[eiselems]

@jhoeller maybe because I did not knew better - but maybe you can help me there.

To give you context:

I was using Spring AMQP and wanted to use a DTO in my MessageListener's receive method.
Our listener is using a retry pattern for retries after 1 hour (without the included spring-retry since this is blocking the listener.)

At the beginning we had the logic to not requeue a message after a specified number of failed retries in the listener. This worked fine as long as the exception was inside the listener.

When there was a message which failed to get parsed into the DTO, the flow exits before entering the method - in effect not checking the retry count, creating an endless requeue loop.

To tackle this I tried to build an advice around the receiveMethod.
(Maybe I just should have used plain AOP here).

What I did instead, I used https://docs.spring.io/spring-amqp/api/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.html#setAdviceChain-org.aopalliance.aop.Advice...- to set the advice on the MessageListenerContainer.

The method has a parameter of type Advice. I passed an implementation of MethodInterceptor here since that seemed to be the most logical thing there.

I would be also be open for a better way to do that - "use Spring AOP directly" would be probably also an idea.

TLDR: I tried to use https://docs.spring.io/spring-amqp/api/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.html#setAdviceChain-org.aopalliance.aop.Advice...- to create in effect a before advice (or an around advice which is not implementing the after part). In order to implement the Advice interface, I have chosen to implement MethodInterceptor (which uses MethodInvocation as parameter).

Hope you got a better picture and it makes at least some sense.

[jhoeller]

Ah I guess I've been missing the rather obvious case where you'd implement a MethodInterceptor and have to call MethodInvocation.proceed() within its body. The problem is therefore when calling proceed() from a Kotlin-based interceptor implementation, not when implementing the invocation chain itself (where we see it ourselves as a nullability mismatch). Thanks for the clarification!

As a workaround for the time being, you could cast the passed-in MethodInvocation to ReflectiveMethodInvocation and call proceed() on the downcast where it is annotated with @Nullable already.