Avoid rollback after a commit failure in `TransactionalOperator` #27572

EnricSala · 2021-10-18T10:32:28Z

A failure to commit a reactive transaction will complete the transaction and clean up resources. Executing a rollback at that point is invalid, which causes an IllegalTransactionStateException that masks the cause of the commit failure.

This change restructures TransactionalOperatorImpl and ReactiveTransactionSupport to avoid executing a rollback after a failed commit. While there, the Mono transaction handling in TransactionalOperator is simplified by moving it to a default method on the interface.

See gh-27523

EnricSala

Left a couple of comments to explain some changes.

EnricSala · 2021-10-18T10:34:13Z

spring-tx/src/main/java/org/springframework/transaction/reactive/TransactionalOperator.java

+	default <T> Mono<T> transactional(Mono<T> mono) {
+		return execute(it -> mono).singleOrEmpty();


A similar default implementation existed in the past but using .next() instead of .singleOrEmpty(). This was changed in gh-23562 because .next() triggers the cancellation of the source.

I believe we could use .singleOrEmpty() which IIUC should not cancel the source before receiving the completion signal from the Mono.

Please let me know if this exceeds the scope of the issue and I would drop it from the PR.

EnricSala · 2021-10-18T10:34:51Z

spring-tx/src/main/java/org/springframework/transaction/reactive/TransactionalOperatorImpl.java

-			// This is an around advice: Invoke the next interceptor in the chain.
-			// 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.flatMapMany(it -> Flux
-					.usingWhen(
-							Mono.just(it),


I was hesitant about filing a PR because I wasn't sure I was fully understanding this part.

IIUC we could now simplify this because the rollbackOnException has been moved to inside the usingWhen.

Relates to this comment: https://github.com/spring-projects/spring-framework/pull/23562/files#r319925489

The previous code commits the transaction depending on successful/exceptional completion/cancellation. Any failure in the async cleanup methods (specifically commit because rollback happens upon cancellation) dropped into rollbackOnException which performs another round of cleanup and that is the actual bug.

To solve the issue, we need to call rollbackOnException in the transaction cleanup instead of onErrorResume(…) and (tx, ex) -> Mono.empty(). That would align the flow with the imperative TransactionTemplate.

It is possible that we need to refine the actual exception if it is emitted by the async clean up afterward.

Note that the same issue exists in TransactionAspectSupport.ReactiveTransactionSupport.

Thanks for the feedback @mp911de :) Do you mean something like this?

Flux.usingWhen( this.transactionManager.getReactiveTransaction(this.transactionDefinition), action::doInTransaction, this.transactionManager::commit, this::rollbackOnException, this.transactionManager::rollback))

Was indeed going for something like this initially but as you mentioned it would require refining the exception, because a failure on either asyncComplete or asyncError wraps the exception like this:

java.lang.RuntimeException: Async resource cleanup failed after onComplete|onError

Maybe that's ok? But that's why I thought that the .onErrorResume(rollback) + .concatWith(commit) combination may achieve something similar and avoid the exception refining. Do you think it would be preferable to go with the asyncComplete&asyncError + exception cleanup approach?

Note that the same issue exists in TransactionAspectSupport.ReactiveTransactionSupport.

Good point! Shall I apply these changes also over there, or do you think separate PR?

That bit looks decent. The more operators we use the more performance impact we can generate. Regarding exception mapping, I'm not sure which top-level exception should be propagated downstream. Looking into the transaction manager, a RuntimeException seems fine. Maybe @jhoeller can provide a bit more guidance.

Shall I apply these changes also over there, or do you think separate PR?

The issue is the same and it makes sense to fix the same problem in multiple places at once as we can keep the context within one commit from your side.

Pushed the changes. Went ahead with a message-based approach to the unwrapping, maybe there is a utility for this? 🤔 Didn't find anything in reactor.core.Exceptions.

Paging @simonbasle, maybe Simon can give further insights.

there isn't such an unwrapping utility currently, not for the sake of detecting that particular kind of exception.

@EnricSala note that in the case of a rollback which fails, we have a RuntimeException with:

the cause of the rollback failure as the getCause()

the original exception in the usingWhen which triggered the rollback in the first place (the rollbackCause) as a suppressed exception

Thanks for the reply! Agree, the exception propagated by usingWhen meets the criteria so leaving it untouched would be an option 👍

I squashed the changes on the branch leaving only two commits: the first one shows the implementation with exception unwrapping and the second commit shows what it would look like if we drop the unwrapping.

Both options resolve my problem, so I think at this point it may be a matter of preference or consistency with the rest of the framework. Please let me know which option would be preferable :)

It makes sense to unwrap the exception as callers typically do not expect a generic RuntimeException.

I would also have a preference for this option because the RuntimeException feels a bit synthetic, it's only there due to the usingWhen implementation, and it doesn't align with the behavior of non-reactive transactions.

I have applied the change, it should be ready for review :)

...ng-tx/src/test/java/org/springframework/transaction/reactive/TransactionalOperatorTests.java

sdeleuze · 2022-09-23T12:21:46Z

@mp911de Could you please review the latest version of this PR and confirm (or not) you are ok for merging it (I will also have a deeper look after your confirmation)?

sdeleuze · 2023-03-01T16:55:42Z

@EnricSala I have rebased the branch on top of main in https://github.com/sdeleuze/spring-framework/tree/gh-27523, but I see an error in CoroutinesTransactionInterceptorTests, could you please have a look and maybe rebased this PR on top of main potentially using my branch as a basis?

This reverts commit aaad051.

EnricSala · 2023-03-02T07:14:04Z

@sdeleuze I have rebased this PR on top of main and added a commit to fix CoroutinesTransactionInterceptorTests. Please check if this small fix makes sense :)

sdeleuze · 2023-03-06T09:09:28Z

Merged via edf0ae7 after @simonbasle and @mp911de green light. Thanks for contributing this and for your patience @EnricSala. Please test snapshots to check everything looks fine for your use cases.

This commit fixes a regression introduced in conjunction with gh-27572. See gh-30597 Closes gh-30729

A failure to commit a reactive transaction will complete the transaction and clean up resources. Executing a rollback at that point is invalid, which causes an IllegalTransactionStateException that masks the cause of the commit failure. This change restructures TransactionalOperatorImpl and ReactiveTransactionSupport to avoid executing a rollback after a failed commit. While there, the Mono transaction handling in TransactionalOperator is simplified by moving it to a default method on the interface. Closes spring-projectsgh-27572

This commit fixes a regression introduced in conjunction with spring-projectsgh-27572. See spring-projectsgh-30597 Closes spring-projectsgh-30729

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 18, 2021

EnricSala commented Oct 18, 2021

View reviewed changes

EnricSala force-pushed the gh-27523 branch from d028b9c to 8a67aca Compare October 23, 2021 14:41

EnricSala force-pushed the gh-27523 branch from 363e7fc to 69a02b2 Compare October 31, 2021 12:06

rstoyanchev added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Nov 10, 2021

sdeleuze self-assigned this Sep 23, 2022

sdeleuze added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 23, 2022

sdeleuze added this to the 5.3.24 milestone Sep 23, 2022

sdeleuze removed this from the 5.3.24 milestone Sep 23, 2022

sdeleuze mentioned this pull request Sep 23, 2022

TransactionalOperator should not attempt to rollback after a failed commit #27523

Closed

simonbasle approved these changes Feb 28, 2023

View reviewed changes

sdeleuze added this to the 6.0.6 milestone Mar 1, 2023

sdeleuze modified the milestones: 6.0.6, 6.0.7 Mar 1, 2023

EnricSala added 4 commits March 2, 2023 07:14

Option 1: unwrap the RuntimeException from usingWhen

0881f09

Option 2: drop the unwrapping, propagate RuntimeException

69fd5d5

Revert "Option 2: drop the unwrapping, propagate RuntimeException"

e7f9cb8

This reverts commit aaad051.

Fix test CoroutinesTransactionInterceptorTests

5f5d68f

EnricSala force-pushed the gh-27523 branch from 69a02b2 to 5f5d68f Compare March 2, 2023 06:56

sdeleuze closed this in edf0ae7 Mar 6, 2023

jhoeller mentioned this pull request Jun 7, 2023

NPE in ReactiveTransactionSupport.unwrapIfResourceCleanupFailure #30597

Closed

sbrannen added a commit that referenced this pull request Jun 23, 2023

Ignore null message when introspecting resource cleanup failure

9ccbeec

This commit fixes a regression introduced in conjunction with gh-27572. See gh-30597 Closes gh-30729

sbrannen mentioned this pull request Aug 28, 2023

Fix Transaction already completed issue #29065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid rollback after a commit failure in `TransactionalOperator` #27572

Avoid rollback after a commit failure in `TransactionalOperator` #27572

EnricSala commented Oct 18, 2021 •

edited

EnricSala left a comment

EnricSala Oct 18, 2021

EnricSala Oct 18, 2021

mp911de Oct 19, 2021

EnricSala Oct 19, 2021

mp911de Oct 19, 2021

EnricSala Oct 19, 2021

mp911de Oct 20, 2021

simonbasle Oct 20, 2021

EnricSala Oct 23, 2021

mp911de Oct 25, 2021

EnricSala Nov 2, 2021

sdeleuze commented Sep 23, 2022

sdeleuze commented Mar 1, 2023

EnricSala commented Mar 2, 2023

sdeleuze commented Mar 6, 2023

		default <T> Mono<T> transactional(Mono<T> mono) {
		return execute(it -> mono).singleOrEmpty();

Avoid rollback after a commit failure in TransactionalOperator #27572

Avoid rollback after a commit failure in TransactionalOperator #27572

Conversation

EnricSala commented Oct 18, 2021 • edited

EnricSala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdeleuze commented Sep 23, 2022

sdeleuze commented Mar 1, 2023

EnricSala commented Mar 2, 2023

sdeleuze commented Mar 6, 2023

Avoid rollback after a commit failure in `TransactionalOperator` #27572

Avoid rollback after a commit failure in `TransactionalOperator` #27572

EnricSala commented Oct 18, 2021 •

edited