spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.55k stars 38.13k forks source link

Spring AOP is not compatible with Kotlin Coroutines #22462

Closed qiyuey closed 1 year ago

qiyuey commented 5 years ago
  1. the suspend fun may return COROUTINE_SUSPENDED
  2. Spring AOP needs to filter COROUTINE_SUSPENDED because the fun did not actually return
sdeleuze commented 5 years ago

Seems to be a pre-requisite for #23575. Good candidate for Spring Framework 5.3.

mkopylec commented 4 years ago

I need this feature badly :)

zymen commented 4 years ago

Hi, also highly requested!

wrotycz commented 4 years ago

Yeah, that would help a lot!

faderskd commented 4 years ago

Need this feature too !

gwieczorek-it commented 4 years ago

Hi, I would love to see this feature implemented.

rudykocur commented 4 years ago

Sadly stumbled upon this bug :( I would need that badly

sdeleuze commented 4 years ago

Hey, here is an update on that topic. I have added @Transactional support for Coroutines via https://github.com/spring-projects/spring-framework/commit/5429c7afebaa3255ea80197224023c29c7d552ec and I have provided some guidance on Spring Security side for similar support.

Those Spring Data and Spring Security supports are possible without any change on Spring AOP, and Coroutines support requires to be added at a Reactive-aware level which is not the case of Spring AOP if I am not mistakes, so I am not sure anymore this issue is relevant.

With this progresses in mind, please share your use cases requiring an update on Spring AOP if any.

sdeleuze commented 4 years ago

I created this draft commit to add Coroutines support for Spring AOP, it is pretty straightforward by itself but I am not yet convinced we should integrate it in 5.3 because:

RobertHeim commented 3 years ago

Any updates?

gs-husam commented 3 years ago

Just hit this as well trying to add @Transactional to suspend functions, would be very helpful to have!

Has anyone found a workaround for @Transactional, apart from wrapping the code in explicit reactive transaction operator blocks?

sdeleuze commented 3 years ago

As documented here:

Transactions on Coroutines are supported via the programmatic variant of the Reactive transaction management provided as of Spring Framework 5.2.

Thread-bound transactions are not supported.

mkopylec commented 3 years ago

When it is planned to be done?

mkopylec commented 3 years ago

Here is a sample of how it can be done. Haven't checked it but looks promising.

renatomrcosta commented 2 years ago

Hi, just noticed this issue when working with @Timed / @NewSpan annotations for Micrometer's TimedAspect and @NewSpan in Sleuth. In both cases, the measurements are erroneously finished at the first suspension point.

Looking forward to updates on this issue as well, and a big thanks to everyone doing work on it!

jzheaux commented 2 years ago

Spring Security needs this in order to support method security in co-routines. The existing method interceptor, PrePostAdviceMethodInterceptor, uses CoroutineUtils.invokeSuspendFunction; however, this has the notable downside of skipping downstream method interceptors.

akhilpratap1991 commented 2 years ago

Here is a sample of how it can be done. Haven't checked it but looks promising.

Anyone able to make this work? I am getting into infinite loop

sdeleuze commented 1 year ago

We were able to leverage Spring AOP + Coroutines in #29527 via the invocation of MonoKt.awaitSingleOrNull with explicit passing of the contination here.

@jzheaux Could you check if this kind of solution is possible on your Spring Security use case? If yes, we can maybe explore if a generic handling of Coroutines with Spring AOP is possible.

andydenk commented 1 year ago

Is this expected to be addressed for Spring 5.3.x / 5.x.x as well?

I am facing this issue and it keeps me from introducing AOP advice to properly trace suspend functions and functions returning flow as OpenTelemetry also seems to lack proper support for coroutines.

sdeleuze commented 1 year ago

No, this is unlikely to be fixed on Spring Framework 5.x.

@jzheaux Could you please provide me your feedback on this comment?

nenros commented 1 year ago

I would ask if there is chance to fix it at all? Issue is really old and there isn't any visible progress here.

sdeleuze commented 1 year ago

This is a tricky one from a design perspective, but let see if we can move forward. I will reach Spring Security team to try to get a feedback.

RobertHeim commented 1 year ago

any updates? seems to block a lot of other issues downstream

jun-wu-tw commented 1 year ago

any updates? seems to block a lot of other issues downstream

Hey, RoberHeim. I find a workaround for coroutine with Aspectj, but I don't remember the link. You can refer to the following code. Hope it can help you out. 🙏

  1. Add the extensions
    
    val ProceedingJoinPoint.coroutineContinuation: Continuation<Any?>
    get() = this.args.last() as Continuation<Any?>

val ProceedingJoinPoint.coroutineArgs: Array<Any?> get() = this.args.sliceArray(0 until this.args.size - 1)

suspend fun ProceedingJoinPoint.proceedCoroutine( args: Array<Any?> = this.coroutineArgs ): Any? = suspendCoroutineUninterceptedOrReturn { continuation -> this.proceed(args + continuation) }

fun ProceedingJoinPoint.runCoroutine( block: suspend () -> Any? ): Any? = block.startCoroutineUninterceptedOrReturn(this.coroutineContinuation)

2. Add the Continuation point cut
```kotlin
    @Pointcut("args(.., kotlin.coroutines.Continuation)")
    fun suspendFunctionPointCut() {

    }
  1. Usage (combine your personal with Continuation point cut)
    @Around("YourCustomPointCut() && suspendFunctionPointCut()")
    fun handleGenericSystemErrors(joinPoint: ProceedingJoinPoint): Any? {
        return joinPoint.runCoroutine {
            val result = joinPoint.proceedCoroutine()!!
            return@runCoroutine result
        }
    }
RobertHeim commented 1 year ago

@jun-wu-tw the problem is not that we need it in our code base, but it must be integrated in spring itself to solve downstream issues:

sdeleuze commented 1 year ago

After working on the Spring Security repro created by @jzheaux, I may have found a potential way to handle this.

I managed to fix the repro with some changes on Spring Framework side that you can see in this draft commit. In a nutshell, at InvocableHandlerMethod level, when a proxified suspending function is detected, we just invoke the regular Java code path with method.invoke(getBean(), args) and defer the invocation of CoroutinesUtils#invokeSuspendingFunction to org.springframework.aop.support.AopUtils#invokeJoinpointUsingReflection.

That way, proxified suspending functions return their Mono counterpart, allowing MethodInterceptors to deal with those like if it was reactive methods returning Mono. This slightly changes how Coroutines interceptors work, but they were broken for a lot of use cases and I think this also makes them more consistent with the rest of the Coroutines support in Spring.

It is possible to test this Spring Framework branch by cloning it, running ./gradlew publishToMavenLocal and then using this repro branch that should now work correctly.

As a consequence, I tentatively plan resolving this issue in Spring Framework 6.1.0-RC1. Since a lot of Spring developers are waiting for the resolution of this issue, please provide your feedback on this proposal if possible.

@jhoeller @rstoyanchev @poutsma @jzheaux Please let me know if you have any concern about the proposed solution.

sdeleuze commented 1 year ago

Looks like we are going to have to do something more evolved in order to support non reflective use cases and make this less intrusive from the caller perspective.

sdeleuze commented 1 year ago

Initial support pushed, please test Spring Framework 6.1.0-SNAPSHOT and let me know how it goes.

@jzheaux Please let me know how it goes on Spring Security after you refine the support to avoid the multiple subscription issues we discussed on your repro.

@mp911de @rstoyanchev I refined reactive transactional and HTTP service interface support to leverage those generic capabilities, tests are green but please ping me if you see related reports on Coroutines support.

tonnoz commented 4 months ago

@jun-wu-tw sir, you saved the day, thank you so much