Propagate `CoroutineContext` in reactive transaction #27308

ks-yim · 2021-08-21T11:47:04Z

Motivation:

Fixes Reactive Transaction when applied to Kotlin Coroutines doesn't pass the CoroutineContext #27307

Modifications:

Propagate caller's CoroutineContext when converting coroutines into Mono.
- In CoroutinesUtils#invokeSuspendingFunction
- In TransactionalOperatorExtensions
  - Flow<T>.transactional(...)
  - TransactionalOperator.executeAndAwait(...)
Change TransactionalOperator.executeAndAwait(...)'s type parameter upper bound from Any to Any?
- For better usability.

pivotal-cla · 2021-08-21T11:47:06Z

@ks-yim Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2021-08-21T11:49:55Z

@ks-yim Thank you for signing the Contributor License Agreement!

sdeleuze

I will have a deeper look to the implementation, but looks like we have something to fix here indeed.

sdeleuze · 2022-03-09T10:23:20Z

spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java

@@ -71,7 +75,9 @@
 	public static Publisher<?> invokeSuspendingFunction(Method method, Object target, Object... args) {
 		KFunction<?> function = Objects.requireNonNull(ReflectJvmMapping.getKotlinFunction(method));
 		KClassifier classifier = function.getReturnType().getClassifier();
-		Mono<Object> mono = MonoKt.mono(Dispatchers.getUnconfined(), (scope, continuation) ->
+		Continuation<?> cont = (Continuation<?>) args[args.length - 1];


I am not super confortable doing this kind of manual handling of the continuation, so I have asked guidance to Kotlin team to see if there is a better way, waiting there feedback.

Feedback from Kotlin team:

What puzzles me is why there is Job in the context.
When you have one, it means that the lifecycle of the result is just incorrect — it is controlled solely by mono, but not by the Job as the caller might’ve expected.
It’s hard to tell if it is exactly wrong without knowing why Job is there in the first place

Potentially related to spring-projects/spring-data-commons#2532 cc @mp911de.

@sdeleuze Sorry for the late feedback and thanks for taking your time to review this.

The reason why it had a Job in the context in my use case is because the async server framework that I am using(Armeria) resorts to GlobalScope#future to bridge its CompletableFuture based implementations to the coroutine world.

I understand the concerns of Kotlin team worrying that the coroutine's lifecycle is no longer be managed by the caller's Job context, but I thought it's okay to do this for this specific use case since the lifecycle control of the returned Mono goes back into the caller's Job context anyways in the subsequent call for awaitSingleOrNull in this line.

Maybe the issue is in Armeria's implementation and there won't be a Job to be stripped away with WebFlux, but my current guess is that it won't be any different with WebFlux. I will run a test and share the result.

It is indeed the same for WebFlux. I will provide your feedback to the Kotlin team, may be indeed ok for this specific use case.

@qwwdfsad Could you please check @ks-yim previous comment, especially this quote below, to help us analyzing if removing Job.Key from the CoroutineContext is ok for this very specific use case where we intercept the suspending invocation to plug our Reactive transaction management and later invoke AwaitKt.awaitSingleOrNull with the original continuation:

I understand the concerns of Kotlin team worrying that the coroutine's lifecycle is no longer be managed by the caller's Job context, but I thought it's okay to do this for this specific use case since the lifecycle control of the returned Mono goes back into the caller's Job context anyways in the subsequent call for awaitSingleOrNull in this line.

FWIW our tests are green with this implementation proposed in my refined commit.

(Another guy from the coroutines project here) You are doing very fancy things here! It did take me a while to understand them. If the CompletableFuture gets cancelled, it will cancel awaitSingleOrNull; if awaitSingleOrNull gets cancelled, it will cancel the mono; if mono completes with an error or a value, it will return that via awaitSingleOrNull. So, in effect, you're getting the same job control with extra steps.

The only issue I see is coroutine code that explicitly does something with the Job, like currentCoroutineContext()[Job]!!.invokeOnCompletion { println("ok") }. If you intercept such code in a mono, then, naturally, the Job used will be the one created by mono, not the Job from future. I don't see a way around this, though.

.../src/main/kotlin/org/springframework/transaction/reactive/TransactionalOperatorExtensions.kt

sdeleuze · 2022-03-15T08:35:05Z

Moving this issue back to waiting for triage while we are waiting more information on spring-projects/spring-data-commons#2532 side.

spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java

jamesbassett · 2022-08-17T23:13:32Z

Are there any updates on this issue? It's a bit of a roadblock for us - we rely on the context being propagated so can't use transactions at the moment :(

sdeleuze · 2022-08-19T08:11:42Z

@jamesbassett Any chance you could provide a repro since we have no feedback on that on the Spring Data issue?

jwmaher · 2022-09-19T08:06:56Z

@sdeleuze I have created a reproducible example here https://github.com/JoeMaher/tx-coroutine-context-loss-example, please let me know if you have any queries/concerns

sdeleuze · 2022-09-23T11:29:39Z

Great, thanks, I will have a look shortly.

jamesbassett · 2022-10-04T07:40:01Z

Hi @sdeleuze did you get a chance to look at @JoeMaher 's reproducer? 🤞

sdeleuze · 2022-10-05T08:29:22Z

Sorry for the delay but I am currently focused on the latest bits of native support for Spring Framework 6 RC1.

jamesbassett · 2023-01-10T23:57:56Z

Hi @sdeleuze just bumping this (now that Spring 6 is released!)

Closes spring-projectsgh-27308

sdeleuze · 2023-02-02T14:07:45Z

Thanks for your patience all.

I re-worked the PR after fixing the nullability issue via #29919 with this commit, I now need to have the confirmation from the Kotlin team this is ok to remove Job.Key from the Coroutines context for this specific use case.

If validated, we may backport the fix to 5.3.x.

sdeleuze · 2023-02-06T10:40:47Z

@mp911de @poutsma Could you please have a look to this commit and this comment from Kotlin team to check if you see something against merging this on main and 5.3.x.

The limitation on Job is coming from Coroutines implementation, looks like a pretty unusual use case, and given the fact that this popular use case is broken for months, merging this will a a significant improvement for a lot of users.

poutsma · 2023-02-06T11:36:48Z

@mp911de @poutsma Could you please have a look to this commit and this comment from Kotlin team to check if you see something against merging this on main and 5.3.x.

I did look, and can follow along with the discussion, but this is too far out of my area of expertise to give any insightful comment. If the PR resolves #27307, and does not break any existing code, than it seems good to me.

sdeleuze · 2023-02-13T14:39:45Z

The fix and tests for both functional and annotation variants are now merged in main and will be available as of Spring Framework 6.0.5 and Spring Boot 3.0.3.

I evaluated if it is possible to merge it on 5.3.x but this is not straightforward as it would require to backport 2 other changes that are impacting the public APIs, so I chose to not backport it.

jamesbassett · 2023-02-13T22:30:11Z

That's fantastic news @sdeleuze thank you so much for progressing this 🥳 I'll give it a test drive when Boot 3.0.3 drops (I imagine that's later this month?)

nkonev · 2023-02-13T23:26:58Z

@jamesbassett there is the calendar with Spring Ecosystem releases https://calendar.spring.io/

jamesbassett · 2023-03-06T06:03:54Z

Just dropping in to say thanks @sdeleuze I tested out Boot 3.0.3 today and can confirm that everything now works as expected (we're not losing context when using transactions) and executeAndAwait() doesn't require the !! (hold-my-beer operator) any more 🥳 Thanks for all your help ❤️

spring-projects-issues added the status: waiting-for-triage label Aug 21, 2021

ks-yim force-pushed the reactive-transaction-coro-ctx branch from b7498d6 to 10d600a Compare August 21, 2021 12:13

sbrannen added in: data type: enhancement labels Aug 22, 2021

sbrannen mentioned this pull request Aug 22, 2021

Reactive Transaction when applied to Kotlin Coroutines doesn't pass the CoroutineContext #27307

Closed

sbrannen requested a review from sdeleuze August 22, 2021 10:43

Propagate CoroutineContext in reactive transaction

09c64ae

ks-yim force-pushed the reactive-transaction-coro-ctx branch from 10d600a to 09c64ae Compare September 18, 2021 14:48

sdeleuze added type: bug and removed status: waiting-for-triage type: enhancement labels Dec 17, 2021

sdeleuze added this to the 5.3.15 milestone Dec 17, 2021

sdeleuze reviewed Dec 17, 2021

View reviewed changes

sdeleuze modified the milestones: 5.3.15, 5.3.16 Jan 12, 2022

sdeleuze added the theme: kotlin label Jan 19, 2022

mp911de mentioned this pull request Jan 20, 2022

Why RepositoryMethodInvoker.doInvokeReactiveToSuspended strips coroutine continuation? spring-projects/spring-data-commons#2532

Open

snicoll modified the milestones: 5.3.16, 5.3.17 Feb 16, 2022

sdeleuze reviewed Mar 9, 2022

View reviewed changes

sdeleuze removed this from the 5.3.17 milestone Mar 15, 2022

sdeleuze added the status: waiting-for-triage label Mar 15, 2022

wine-area reviewed May 24, 2022

View reviewed changes

spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java Show resolved Hide resolved

psimonsked approved these changes May 27, 2022

View reviewed changes

sdeleuze mentioned this pull request Feb 2, 2023

Loosing Kotlin coroutine context when using @Transactional annotation #28290

Closed

sdeleuze removed the status: waiting-for-triage label Feb 2, 2023

sdeleuze added this to the 6.0.x milestone Feb 2, 2023

sdeleuze self-assigned this Feb 2, 2023

sdeleuze added a commit to sdeleuze/spring-framework that referenced this pull request Feb 2, 2023

Propagate the context in Coroutines transactions

6694bda

Closes spring-projectsgh-27308

sdeleuze modified the milestones: 6.0.x, 6.0.5 Feb 2, 2023

sdeleuze closed this in 45ae00f Feb 13, 2023

ks-yim deleted the reactive-transaction-coro-ctx branch February 14, 2023 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate `CoroutineContext` in reactive transaction #27308

Propagate `CoroutineContext` in reactive transaction #27308

ks-yim commented Aug 21, 2021 •

edited

Loading

pivotal-cla commented Aug 21, 2021

pivotal-cla commented Aug 21, 2021

sdeleuze left a comment

sdeleuze Mar 9, 2022

sdeleuze Mar 15, 2022

ks-yim May 25, 2022 •

edited

Loading

sdeleuze Feb 2, 2023

sdeleuze Feb 2, 2023

dkhalanskyjb Feb 3, 2023

sdeleuze commented Mar 15, 2022

jamesbassett commented Aug 17, 2022

sdeleuze commented Aug 19, 2022

jwmaher commented Sep 19, 2022

sdeleuze commented Sep 23, 2022

jamesbassett commented Oct 4, 2022

sdeleuze commented Oct 5, 2022

jamesbassett commented Jan 10, 2023

sdeleuze commented Feb 2, 2023

sdeleuze commented Feb 6, 2023

poutsma commented Feb 6, 2023

sdeleuze commented Feb 13, 2023 •

edited

Loading

jamesbassett commented Feb 13, 2023

nkonev commented Feb 13, 2023

jamesbassett commented Mar 6, 2023

Propagate CoroutineContext in reactive transaction #27308

Propagate CoroutineContext in reactive transaction #27308

Conversation

ks-yim commented Aug 21, 2021 • edited Loading

Motivation:

Modifications:

pivotal-cla commented Aug 21, 2021

pivotal-cla commented Aug 21, 2021

sdeleuze left a comment

Choose a reason for hiding this comment

sdeleuze Mar 9, 2022

Choose a reason for hiding this comment

sdeleuze Mar 15, 2022

Choose a reason for hiding this comment

ks-yim May 25, 2022 • edited Loading

Choose a reason for hiding this comment

sdeleuze Feb 2, 2023

Choose a reason for hiding this comment

sdeleuze Feb 2, 2023

Choose a reason for hiding this comment

dkhalanskyjb Feb 3, 2023

Choose a reason for hiding this comment

sdeleuze commented Mar 15, 2022

jamesbassett commented Aug 17, 2022

sdeleuze commented Aug 19, 2022

jwmaher commented Sep 19, 2022

sdeleuze commented Sep 23, 2022

jamesbassett commented Oct 4, 2022

sdeleuze commented Oct 5, 2022

jamesbassett commented Jan 10, 2023

sdeleuze commented Feb 2, 2023

sdeleuze commented Feb 6, 2023

poutsma commented Feb 6, 2023

sdeleuze commented Feb 13, 2023 • edited Loading

jamesbassett commented Feb 13, 2023

nkonev commented Feb 13, 2023

jamesbassett commented Mar 6, 2023

Propagate `CoroutineContext` in reactive transaction #27308

Propagate `CoroutineContext` in reactive transaction #27308

ks-yim commented Aug 21, 2021 •

edited

Loading

ks-yim May 25, 2022 •

edited

Loading

sdeleuze commented Feb 13, 2023 •

edited

Loading