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 coroutines AOP is not compatible with @Transactional #33095

Closed
backtony opened this issue Jun 25, 2024 · 5 comments
Closed

Spring coroutines AOP is not compatible with @Transactional #33095

backtony opened this issue Jun 25, 2024 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support type: bug A general bug
Milestone

Comments

@backtony
Copy link

backtony commented Jun 25, 2024

spring boot version 3.3.0

When using Spring Coroutine AOP, if the @transactional annotation and a custom AOP are applied together without specifying an order in the Aspect, there is an issue where the custom AOP does not get applied.

@Service
class TargetService() {
    private val log = KotlinLogging.logger { }

    @Logging
    @Transactional
    suspend fun aop(): String {
        delay(100)
        log.info { "aop target method call" }

        return "ok"
    }
}
@Aspect
//@Order(1)
@Component
class LoggingAspect {

    private val log = KotlinLogging.logger {}

    @Around("@annotation(com.example.aopwithtransaction.aop.Logging)")
    fun logging(joinPoint: ProceedingJoinPoint): Any? {
        return mono {
            log.info { "Aop Logging started" }

            val result = joinPoint.proceed().let { result ->
                if (result is Mono<*>) {
                    result.awaitSingleOrNull()
                } else {
                    result
                }
            }

            log.info { "Aop Logging completed" }

            result
        }
    }
}

In cases like the one above, the custom AOP does not function unless the order is specified, in which case it does function.

This is a simple example : https://github.com/backtony/spring-reactive-aop-transaction

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 25, 2024
@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Jun 25, 2024
@sdeleuze sdeleuze added the theme: kotlin An issue related to Kotlin support label Jun 26, 2024
@sdeleuze sdeleuze self-assigned this Jun 26, 2024
@sdeleuze
Copy link
Contributor

It is likely coming from the fact using mono {  } loses the CoroutineContext. I would suggest to write your aspect with Reactive operators if you are not familiar with those advanced concepts, for example this should work even without @Order(1):

@Around("@annotation(com.example.aopwithtransaction.aop.Logging)")
fun logging(joinPoint: ProceedingJoinPoint): Any? {
    log.info { "Aop Logging started" }
    return (joinPoint.proceed(joinPoint.args) as Mono<*>).doOnTerminate {
        log.info { "Aop Logging completed" }
    }
}

As a consequence, I am closing the issue as invalid.

@sdeleuze sdeleuze closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2024
@sdeleuze sdeleuze added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 26, 2024
@backtony
Copy link
Author

backtony commented Jun 26, 2024

@sdeleuze
Thank you for your response.

@Around("@annotation(com.example.aopwithtransaction.aop.Logging)")
fun logging(joinPoint: ProceedingJoinPoint): Any? {
    log.info { "Aop Logging started" }
    return (joinPoint.proceed(joinPoint.args) as Mono<*>).doOnTerminate {
        log.info { "Aop Logging completed" }
    }
}

Even when using the code you provided, applying both @transactional and @logging annotations on a method results in only @transactional being applied, and the @logging AOP is not applied.

The @logging AOP only works when @order is also added to the code you provided.

Is this really an issue related to the use of Mono? The same issue occurs even when using coroutines.

@Service
class TargetService() {
    private val log = KotlinLogging.logger { }

    @CoroutineLogging
    @Transactional
    suspend fun coroutineAop(): String {
        delay(100)
        log.info { "aop target method call" }

        return "ok"
    }
}
@Aspect
//@Order(1)
@Component
class LoggingAspect {

    private val log = KotlinLogging.logger {}
    @Around("@annotation(com.example.aopwithtransaction.aop.CoroutineLogging)")
    fun coroutineLogging(joinPoint: ProceedingJoinPoint): Any? {
        return joinPoint.runCoroutine {
            log.info { "coroutine logging" }

            val result = joinPoint.proceedCoroutine().let { result ->
                if (result is Mono<*>) {
                    result.awaitSingleOrNull()
                } else {
                    result
                }
            }

            log.info { "Coroutine logging" }
            result
        }
    }
}
fun ProceedingJoinPoint.runCoroutine(
    block: suspend () -> Any?,
): Any? = block.startCoroutineUninterceptedOrReturn(this.coroutineContinuation())

@Suppress("UNCHECKED_CAST")
fun ProceedingJoinPoint.coroutineContinuation(): Continuation<Any?> {
    return this.args.last() as Continuation<Any?>
}

fun ProceedingJoinPoint.coroutineArgs(): Array<Any?> {
    return 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)
}

@sdeleuze
Copy link
Contributor

I checked locally with:

@GetMapping("/aop")
    suspend fun aop(): String {
        delay(100)
        return targetService.aop().awaitSingle()
    }

And

@Aspect
@Component
class LoggingAspect {

    private val log = KotlinLogging.logger {}

    @Around("@annotation(com.example.aopwithtransaction.aop.Logging)")
    fun logging(joinPoint: ProceedingJoinPoint): Any? {
        log.info { "Aop Logging started" }
        return (joinPoint.proceed(joinPoint.args) as Mono<*>).doOnTerminate {
            log.info { "Aop Logging completed" }
        }
    }
}

And the logging is working.

@backtony
Copy link
Author

@sdeleuze
Thank you for your response.

Since TargetService.aop() is a suspend function, awaitSingle is not possible.

The reason AOP logging is printed in your local environment is probably because the targetService.aop() method did not have @transactional.

When both @transactional and @logging are attached to targetService.aop() as shown below, only @transactional is applied, and AOP for logging is not applied.

Please check the sample I provided again.
https://github.com/backtony/spring-reactive-aop-transaction

@RestController
class AopController(
    private val targetService: TargetService,
) {

    @GetMapping("/aop")
    suspend fun aop(): String {
        return targetService.aop()
    }
}
@Service
class TargetService() {
    private val log = KotlinLogging.logger { }

    @Logging
    @Transactional
    suspend fun aop(): String {
        delay(100)
        log.info { "aop target method call" }

        return "ok"
    }
}
@Aspect
@Component
class LoggingAspect() {

    private val log = KotlinLogging.logger {}

    @Around("@annotation(com.example.aopwithtransaction.aop.Logging)")
    fun logging(joinPoint: ProceedingJoinPoint): Any? {
        log.info { "Aop Logging started" }
        return (joinPoint.proceed(joinPoint.args) as Mono<*>).doOnTerminate {
            log.info { "Aop Logging completed" }
        }
    }
}

result

스크린샷 2024-06-29 오후 6 57 45

@sdeleuze
Copy link
Contributor

sdeleuze commented Jul 3, 2024

Thanks for the updated sample, I can indeed reproduce (only with a suspending function, a function returning Mono<String> works as expected).

@sdeleuze sdeleuze reopened this Jul 3, 2024
@sdeleuze sdeleuze added type: bug A general bug and removed status: invalid An issue that we don't feel is valid labels Jul 3, 2024
@sdeleuze sdeleuze added this to the 6.1.11 milestone Jul 3, 2024
simonbasle added a commit to simonbasle/spring-framework that referenced this issue Jul 10, 2024
Previous to this change, the transactional aspect would supersed the
user-defined AspectJ aspect, shortcircuiting to calling the original
Kotlin suspending function.

This change simplifies the TransactionAspectSupport way of dealing with
transactional coroutines, thanks to the fact that lower level support
for AOP has been introduced in c8169e5.

Closes spring-projectsgh-33095
simonbasle added a commit that referenced this issue Jul 15, 2024
This change simplifies the CacheInterceptor way of dealing with cached
coroutines, thanks to the fact that lower level support for AOP has been
introduced in c8169e5. This fix is similar to the one applied for
`@Transactional` in gh-33095.

Closes gh-33210
@simonbasle simonbasle changed the title Spring coroutines AOP is not compatible with @Transactional Spring coroutines AOP is not compatible with @Transactional Jul 15, 2024
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: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants