-
Notifications
You must be signed in to change notification settings - Fork 37.9k
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
Document how reactive transactions work for cancellation in 5.2 and how it will work in 5.3 #25091
Comments
... instead of committing. This fixes atomicity violation problems. See spring-projects#25091 and https://stackoverflow.com/questions/61822249/spring-reactive-transaction-gets-committed-on-cancel-producing-partial-commits?noredirect=1#comment109381171_61822249
@mp911de @rstoyanchev The affected code dates back to the Reactor Dysprosium RC1 upgrade where we switched to a different |
The code works as designed. Whether commit on cancel is appropriate is an entirely different topic really and depends on the perspective. Cancellation is a pretty standard signal for operators like Operators like From a So what's required is two things:
Cancelation works as per the definition in the Reactive Streams specification. We would require either a Reactor-specific extension or ideally an approach on the Reactive Streams level. Adding @bsideup and @simonbasle to this thread. |
Thanks for the clarification, Mark. I totally missed that a cancellation signal may also indicate the regular end of consumption of an expected number of elements. There really should be some way of indicating cancel-orderly-consumption from cancel-on-failure here, in particular also for the timeout and connection reset cases. Could we potentially track some additional state along with our transaction context here? Since this isn't really obvious, we should at least add documentation around this in 5.2.7. If we can revise semantics in alignment with a Reactor extension here, the Spring Framework 5.3 timeframe would be a fine opportunity for it. Let's create a separate ticket there if we decide to revise this. |
Unfortunately, the Reactive Streams' cancellation indeed does not allow us to indicate the reason or even perform an async cancellation. We could have a mutable "cancellation reason" flag in subscriber's I will bring it on the next team's call to discuss further. |
A comment for application-developers, like me, who need some workaround right now, in hope it may be useful before the issue is sorted out. Indeed, with 'rollback-on-cancel' policy like this rpuch@95c2872, the following code
with
causes a rollback, so nothing is saved. So it's like this:
Please correct me if I'm wrong. |
Team decision: The current behavior (commit on cancel) was added to not interfere with accidental cancellation due to operators such as We figured also that there are several use-cases where commit on cancel is not appropriate:
For Spring Framework 5.2, we're going to document the current behavior to explain semantics. For 5.3, we're going to change the behavior to rollback on cancel. By flipping semantics of cancel signals we create a reliable and deterministic outcome. Cancellations by protective means or as consequence of an error lead to a rollback. These arrangements are typically expensive to test. Although cancellations caused by operators such as We might consider adding an operator to enable commit on cancel semantics if these are cases worth supporting. |
Affects: 5.2.6, current master
Briefly: according to my observations, Spring commits a transaction when a 'transactional reactive pipeline' gets cancelled. To the moment of such a cancellation, it could happen that only part of the work inside the transaction has been done, so a commit at that point produces partially-committed results violating the 'atomicity' transactional property.
I've created a project demonstrating the problem: https://github.com/rpuch/spring-commit-on-cancel-problems
The main (and only) test,
PartialCommitsOnCancelTest#cancelShouldNotLeadToPartialCommit()
, does the following: it initiates a reactive pipeline having 2 inserts. Both inserts are wrapped in a (declarative) transaction. The code is crafted to make a cancellation exactly between the inserts possible:The pipeline is initiated, and after the first insert is done (but before the second one is initiated), the test cancels the pipeline. It then inspects the collection and finds that there is exactly 1 document, which means that the transaction was committed partially.
In the log, I see the following:
So the code is actually run in a transaction. The
savePair()
pipeline never gets completed successfully, but the transaction gets committed producing the 'partial' result.Looking at
TransactionAspectSupport.ReactiveTransactionSupport#invokeWithinTransaction()
, I see the following code:The last parameter is
onCancel
. So it actually commits on cancel. (The same behavior is inTransactionalOperatorImpl
; also,spring-data-mongodb
'sReactiveMongoTemplate#inTransaction()
does the same, but it's a different Spring project).This looks like a bug to me, but I can hardly believe that this behavior was implemented by mistake. Is it possible that I misunderstood something?
PS. There is an SO question with details and some context: https://stackoverflow.com/questions/61822249/spring-reactive-transaction-gets-committed-on-cancel-producing-partial-commits?noredirect=1#comment109381171_61822249
The text was updated successfully, but these errors were encountered: