Duplicate (un-intended) commit with Spring Data R2dbc, causing "Async resource cleanup failed after onComplete" and "Transaction is already completed - do not call commit or rollback more than once per transaction" exceptions #28968
Assignees
Labels
in: data
Issues in data modules (jdbc, orm, oxm, tx)
status: duplicate
A duplicate of another issue
type: bug
A general bug
Comments
spring-projects-issues
added
the
status: waiting-for-triage
62mkv
changed the title
|
There are a couple of things that come together:
The most difficult part was to simplify the reproducer. I'm going to submit a pull request to fix these issues but it can take a while because I'm currently busy with other tasks. |
jhoeller
added
type: bug
|
@mp911de If you get the time, please send a PR with your fix and a related test. |
|
I tried to come up with a patch for this one: Index: spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java b/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java
--- a/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java (revision 63d841664c4b7b11451c25104dc0d66c220ba24f)
+++ b/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java (date 1677686286349)
@@ -433,7 +433,7 @@
AtomicBoolean beforeCompletionInvoked = new AtomicBoolean();
- Mono<Object> commit = prepareForCommit(synchronizationManager, status)
+ Mono<Void> commit = prepareForCommit(synchronizationManager, status)
.then(triggerBeforeCommit(synchronizationManager, status))
.then(triggerBeforeCompletion(synchronizationManager, status))
.then(Mono.defer(() -> {
@@ -445,11 +445,11 @@
return doCommit(synchronizationManager, status);
}
return Mono.empty();
- })).then(Mono.empty().onErrorResume(ex -> {
- Mono<Object> propagateException = Mono.error(ex);
+ })).onErrorResume(ex -> {
+ Mono<Void> propagateException = Mono.error(ex);
// Store result in a local variable in order to appease the
// Eclipse compiler with regard to inferred generics.
- Mono<Object> result = propagateException;
+ Mono<Void> result = propagateException;
if (ErrorPredicates.UNEXPECTED_ROLLBACK.test(ex)) {
result = triggerAfterCompletion(synchronizationManager, status, TransactionSynchronization.STATUS_ROLLED_BACK)
.then(propagateException);
@@ -471,7 +471,7 @@
}
return result;
- })).then(Mono.defer(() -> triggerAfterCommit(synchronizationManager, status).onErrorResume(ex ->
+ }).then(Mono.defer(() -> triggerAfterCommit(synchronizationManager, status).onErrorResume(ex ->
triggerAfterCompletion(synchronizationManager, status, TransactionSynchronization.STATUS_COMMITTED).then(Mono.error(ex)))
.then(triggerAfterCompletion(synchronizationManager, status, TransactionSynchronization.STATUS_COMMITTED))));The erroneous |
|
I'll try to come up with a unit test that can reproduce this, and apply @mp911de 's patch (thanks Mark) |
simonbasle
added a commit
to simonbasle/spring-framework
that referenced
this issue
Mar 9, 2023
This change fixes a situation where error handling was skipped during `processCommit()` in case the `doCommit()` failed. The error handling was set up via an `onErrorResume` operator that was nested inside a `then(...)`, applied to an inner `Mono.empty()`. As a consequence, it would never receive an error signal (effectively decoupling the onErrorResume from the main chain). This change simply moves the error handling back one level up. It also simplifies the `doCommit` code a bit by getting rid of the steps that artificially introduce a `Mono<Object>` return type, which is not really needed. A pre-existing test was missing the fact that the rollback didn't occur, which is now fixed. Another dedicated test is introduced building upon the `ReactiveTestTransactionManager` class. Closes spring-projectsgh-28968 Closes spring-projectsgh-30096
|
I am closing this issue in favor of the PR #30096. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Affects: 5.3.10
I was planning to build a minimal reproducible project for this #28968, but now I have something even stranger, it might also be related to "spring-tx" but I still feel like it should be raised here originally. Because I (almost) never use
spring-txdirectly, so.. Anyway, let me know and I will move the issue to a proper project, if necessary.So, we have a very small integration test: https://github.com/62mkv/spring-cockroach-transaction-retry/tree/duplicate-commit
This code demonstrates really weird (to me) behaviour: it tries to execute "COMMIT" twice, on a same transaction, which brings it first to "no transaction exists" exception and then (it seems) also tries to ROLLBACK, which also causes another exception again, this time from Spring infra.
Here's the logs I get:
the original issue text:
I've already commented under commit diff, although I am not sure if it's visible to anyone, so I decided to post an issue too: b5e5e33#r81307010
In the current
mainversion, that would be question aboutspring-framework/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java
Line 448 in b595dc1
I both can not imagine how that
onErrorResumecould be activated for aMonoEmptypublisher; also I have not been able to trigger any breakpoint inside this lambda, even though I can confirm that code visits both assembly phase parts and lambdas before and after this code.Thanks for checking this out!
Some context: I am fighting a situation where, faced with an exception while COMMIT, the code (instead of retrying transaction, as we would want to, per https://www.cockroachlabs.com/docs/v22.1/transactions.html#client-side-intervention) tries to issue yet another COMMIT, which of course fails and now it's not a retryable exception even.
The text was updated successfully, but these errors were encountered: