Omit cancellation of transactional Monos in TransactionOperator #23562
Conversation
TransactionOperator.as(Mono) now no longer short-cuts via a Flux.next() but provides an implementation via Mono.usingWhen(…). The short-cut previously issued a cancellation signal to the transactional Mono causing the transaction cleanup to happen without a handle for synchronization. Using Mono.usingWhen(…) initiates transaction cleanup when the Mono completes eliminating the need for cancellation of the transactional Publisher. This change does not fully fix spring-projectsgh-23304 but it softens its impact because TransactionalOperator.transactional(Mono) avoids cancellation.
spring-projects-issues
added
the
status: waiting-for-triage
| // This will normally result in a target object being invoked. | ||
| // Need re-wrapping of ReactiveTransaction until we get hold of the exception | ||
| // through usingWhen. | ||
| return status.flatMap(it -> Mono.usingWhen(Mono.just(it), ignore -> mono, |
There was a problem hiding this comment.
wouldn't it be better to start from Mono.usingWhen?
return Mono.usingWhen(status, ignore -> mono, this.transactionManager::commit, ...)There was a problem hiding this comment.
We require the status value outside of usingWhen(…) (see rollbackOnException)
| // Need re-wrapping of ReactiveTransaction until we get hold of the exception | ||
| // through usingWhen. | ||
| return status.flatMap(it -> Mono.usingWhen(Mono.just(it), ignore -> mono, | ||
| this.transactionManager::commit, s -> Mono.empty()) |
There was a problem hiding this comment.
note that in case of cancellation, since no asyncCancel is defined here, it defaults to commit. Also, it feels weird that the error case is basically ignored
There was a problem hiding this comment.
Commit on cancel is what we want if just a subset of data is processed. In imperative code this compares to processing a subset of a result list.
The error case is ignored because errors are handled after the usingWhen operator. In case a pre-commit action fails, we want to issue a rollback. Committing a transaction consists of several activities of which the actual commit is just one of them. So if a commit fails, we still need to rollback,
There was a problem hiding this comment.
makes sense, thanks for the explanations. nothing to add 👍
|
This PR is good to be merged from my side. It would be good to have this fix available in 5.2 GA. |
jhoeller
added
in: data
TransactionOperator.as(Mono)now no longer short-cuts via aFlux.next()but provides an implementation viaMono.usingWhen(…).The short-cut previously issued a cancellation signal to the transactional
Monocausing the transaction cleanup to happen without a handle for synchronization.Using
Mono.usingWhen(…)initiates transaction cleanup when theMonocompletes eliminating the need for cancellation of the transactionalPublisher.This change does not fully fix #23304 but it softens its impact because
TransactionalOperator.transactional(Mono)avoids cancellation./cc @michael-simons @simonbasle @smaldini