spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

Propagate `CoroutineContext` in reactive transaction

Open ks-yim opened this issue 3 years ago • 6 comments

Motivation:

  • Fixes #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.

ks-yim avatar Aug 21 '21 11:08 ks-yim

@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 avatar Aug 21 '21 11:08 pivotal-cla

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

pivotal-cla avatar Aug 21 '21 11:08 pivotal-cla

Moving this issue back to waiting for triage while we are waiting more information on https://github.com/spring-projects/spring-data-commons/issues/2532 side.

sdeleuze avatar Mar 15 '22 08:03 sdeleuze

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 :(

jamesbassett avatar Aug 17 '22 23:08 jamesbassett

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

sdeleuze avatar Aug 19 '22 08:08 sdeleuze

@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

jwmaher avatar Sep 19 '22 08:09 jwmaher

Great, thanks, I will have a look shortly.

sdeleuze avatar Sep 23 '22 11:09 sdeleuze

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

jamesbassett avatar Oct 04 '22 07:10 jamesbassett

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

sdeleuze avatar Oct 05 '22 08:10 sdeleuze

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

jamesbassett avatar Jan 10 '23 23:01 jamesbassett

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 avatar Feb 02 '23 14:02 sdeleuze

@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.

sdeleuze avatar Feb 06 '23 10:02 sdeleuze

@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.

poutsma avatar Feb 06 '23 11:02 poutsma

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.

sdeleuze avatar Feb 13 '23 14:02 sdeleuze

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?)

jamesbassett avatar Feb 13 '23 22:02 jamesbassett

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

nkonev avatar Feb 13 '23 23:02 nkonev

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 ❤️

jamesbassett avatar Mar 06 '23 06:03 jamesbassett