Skip to content
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

Spring AOP is not compatible with Kotlin Coroutines #22462

Closed
Tracked by #11335
qiyuey opened this issue Feb 24, 2019 · 28 comments
Closed
Tracked by #11335

Spring AOP is not compatible with Kotlin Coroutines #22462

qiyuey opened this issue Feb 24, 2019 · 28 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Milestone

Comments

@qiyuey
Copy link

qiyuey commented Feb 24, 2019

  1. the suspend fun may return COROUTINE_SUSPENDED
  2. Spring AOP needs to filter COROUTINE_SUSPENDED because the fun did not actually return
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 24, 2019
@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 9, 2019

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

@sdeleuze sdeleuze added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 9, 2019
@sdeleuze sdeleuze modified the milestones: 5.x Backlog, 5.3 M1 Sep 9, 2019
@jhoeller jhoeller modified the milestones: 5.3 M1, 5.3 M2 Feb 24, 2020
@jhoeller jhoeller modified the milestones: 5.3 M2, 5.3 RC1 Aug 7, 2020
@sdeleuze sdeleuze modified the milestones: 5.3 RC1, 5.3 RC2 Aug 26, 2020
@mkopylec
Copy link

I need this feature badly :)

@zymen
Copy link

zymen commented Sep 10, 2020

Hi, also highly requested!

@wrotycz
Copy link

wrotycz commented Sep 10, 2020

Yeah, that would help a lot!

@faderskd
Copy link

Need this feature too !

@gwieczorek-it
Copy link

Hi, I would love to see this feature implemented.

@rudykocur
Copy link

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

@sdeleuze sdeleuze modified the milestones: 5.3 RC2, 5.3 GA Oct 12, 2020
@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 25, 2020

Hey, here is an update on that topic. I have added @Transactional support for Coroutines via 5429c7a 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 sdeleuze modified the milestones: 6.0.x, 6.1.x Feb 21, 2023
@andydenk
Copy link

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
Copy link
Contributor

sdeleuze commented Jun 20, 2023

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

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

@nenros
Copy link

nenros commented Jun 20, 2023

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
Copy link
Contributor

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
Copy link

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

@jun-wu-tw
Copy link

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)
  1. Add the Continuation point cut
    @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
Copy link

RobertHeim commented Jul 10, 2023

@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
Copy link
Contributor

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 sdeleuze modified the milestones: 6.1.x, 6.1.0-RC1 Aug 17, 2023
@sdeleuze
Copy link
Contributor

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 added a commit to sdeleuze/spring-framework that referenced this issue Aug 25, 2023
This commit adds support for Kotlin Coroutines to Spring AOP
by leveraging CoroutinesUtils#invokeSuspendingFunction in
AopUtils#invokeJoinpointUsingReflection to convert it to the
equivalent Mono return value, like in other parts of Spring
Framework.

That allows MethodInterceptor with Reactive support to process
related return values.

CglibAopProxy#processReturnType and
org.springframework.aop.framework.JdkDynamicAopProxy.invoke
takes care of the conversion from the Publisher return value
to Kotlin Coroutines if needed.

Closes spring-projectsgh-22462
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Aug 25, 2023
This commit adds support for Kotlin Coroutines to Spring AOP
by leveraging CoroutinesUtils#invokeSuspendingFunction in
AopUtils#invokeJoinpointUsingReflection to convert it to the
equivalent Mono return value, like in other parts of Spring
Framework.

That allows MethodInterceptor with Reactive support to process
related return values.

CglibAopProxy#processReturnType and JdkDynamicAopProxy#invoke
takes care of the conversion from the Publisher return value
to Kotlin Coroutines if needed.

Reactive transactional and HTTP service interface support
have been refined to leverage those new generic capabilities.

Closes spring-projectsgh-22462
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Aug 25, 2023
This commit adds support for Kotlin Coroutines to Spring AOP
by leveraging CoroutinesUtils#invokeSuspendingFunction in
AopUtils#invokeJoinpointUsingReflection to convert it to the
equivalent Publisher return value, like in other parts of Spring
Framework.

That allows method interceptors with Reactive support to process
related return values.

CglibAopProxy#processReturnType and JdkDynamicAopProxy#invoke
take care of the conversion from the Publisher return value
to Kotlin Coroutines.

Reactive transactional and HTTP service interface support
have been refined to leverage those new generic capabilities.

Closes spring-projectsgh-22462
@sdeleuze
Copy link
Contributor

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.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests