Skip to content

Commit 2880e6f

Browse files
jhoellerjhoeller
committed
Consistently release savepoint after nested transaction
Closes gh-31133
1 parent 11dc11e commit 2880e6f

File tree

2 files changed

+145
-68
lines changed

2 files changed

+145
-68
lines changed

‎spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/R2dbcTransactionManager.java

Lines changed: 43 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -337,36 +337,36 @@ protected Mono<Void> doCleanupAfterCompletion(TransactionSynchronizationManager
337337
return Mono.defer(() -> {
338338
ConnectionFactoryTransactionObject txObject = (ConnectionFactoryTransactionObject) transaction;
339339

340+
if (txObject.hasSavepoint()) {
341+
// Just release the savepoint, keeping the transactional connection.
342+
return txObject.releaseSavepoint();
343+
}
344+
340345
// Remove the connection holder from the context, if exposed.
341346
if (txObject.isNewConnectionHolder()) {
342347
synchronizationManager.unbindResource(obtainConnectionFactory());
343348
}
344349

345350
// Reset connection.
346-
Connection con = txObject.getConnectionHolder().getConnection();
347-
348-
Mono<Void> afterCleanup = Mono.empty();
349-
350-
Mono<Void> releaseConnectionStep = Mono.defer(() -> {
351-
try {
352-
if (txObject.isNewConnectionHolder()) {
353-
if (logger.isDebugEnabled()) {
354-
logger.debug("Releasing R2DBC Connection [" + con + "] after transaction");
355-
}
356-
Mono<Void> releaseMono = ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory());
357-
if (logger.isDebugEnabled()) {
358-
releaseMono = releaseMono.doOnError(
359-
ex -> logger.debug(String.format("Error ignored during cleanup: %s", ex)));
360-
}
361-
return releaseMono.onErrorComplete();
351+
try {
352+
if (txObject.isNewConnectionHolder()) {
353+
Connection con = txObject.getConnectionHolder().getConnection();
354+
if (logger.isDebugEnabled()) {
355+
logger.debug("Releasing R2DBC Connection [" + con + "] after transaction");
362356
}
357+
Mono<Void> releaseMono = ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory());
358+
if (logger.isDebugEnabled()) {
359+
releaseMono = releaseMono.doOnError(
360+
ex -> logger.debug(String.format("Error ignored during cleanup: %s", ex)));
361+
}
362+
return releaseMono.onErrorComplete();
363363
}
364-
finally {
365-
txObject.getConnectionHolder().clear();
366-
}
367-
return Mono.empty();
368-
});
369-
return afterCleanup.then(releaseConnectionStep);
364+
}
365+
finally {
366+
txObject.getConnectionHolder().clear();
367+
}
368+
369+
return Mono.empty();
370370
});
371371
}
372372

@@ -511,23 +511,36 @@ public boolean isTransactionActive() {
511511
return (this.connectionHolder != null && this.connectionHolder.isTransactionActive());
512512
}
513513

514+
public boolean hasSavepoint() {
515+
return (this.savepointName != null);
516+
}
517+
514518
public Mono<Void> createSavepoint() {
515519
ConnectionHolder holder = getConnectionHolder();
516-
this.savepointName = holder.nextSavepoint();
517-
return Mono.from(holder.getConnection().createSavepoint(this.savepointName));
520+
String currentSavepoint = holder.nextSavepoint();
521+
this.savepointName = currentSavepoint;
522+
return Mono.from(holder.getConnection().createSavepoint(currentSavepoint));
523+
}
524+
525+
public Mono<Void> releaseSavepoint() {
526+
String currentSavepoint = this.savepointName;
527+
if (currentSavepoint == null) {
528+
return Mono.empty();
529+
}
530+
this.savepointName = null;
531+
return Mono.from(getConnectionHolder().getConnection().releaseSavepoint(currentSavepoint));
518532
}
519533

520534
public Mono<Void> commit() {
521-
Connection connection = getConnectionHolder().getConnection();
522-
return (this.savepointName != null ?
523-
Mono.from(connection.releaseSavepoint(this.savepointName)) :
524-
Mono.from(connection.commitTransaction()));
535+
return (hasSavepoint() ? Mono.empty() :
536+
Mono.from(getConnectionHolder().getConnection().commitTransaction()));
525537
}
526538

527539
public Mono<Void> rollback() {
528540
Connection connection = getConnectionHolder().getConnection();
529-
return (this.savepointName != null ?
530-
Mono.from(connection.rollbackTransactionToSavepoint(this.savepointName)) :
541+
String currentSavepoint = this.savepointName;
542+
return (currentSavepoint != null ?
543+
Mono.from(connection.rollbackTransactionToSavepoint(currentSavepoint)) :
531544
Mono.from(connection.rollbackTransaction()));
532545
}
533546

‎spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerUnitTests.java

Lines changed: 102 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
import org.junit.jupiter.api.BeforeEach;
2828
import org.junit.jupiter.api.Test;
2929
import org.mockito.ArgumentCaptor;
30+
import org.mockito.InOrder;
31+
import reactor.core.publisher.Flux;
3032
import reactor.core.publisher.Mono;
3133
import reactor.test.StepVerifier;
3234

@@ -44,6 +46,7 @@
4446
import static org.mockito.ArgumentMatchers.any;
4547
import static org.mockito.ArgumentMatchers.anyBoolean;
4648
import static org.mockito.ArgumentMatchers.anyString;
49+
import static org.mockito.BDDMockito.inOrder;
4750
import static org.mockito.BDDMockito.mock;
4851
import static org.mockito.BDDMockito.never;
4952
import static org.mockito.BDDMockito.reset;
@@ -365,53 +368,110 @@ void testPropagationNeverWithExistingTransaction() {
365368

366369
@Test
367370
void testPropagationNestedWithExistingTransaction() {
368-
when(connectionMock.createSavepoint("SAVEPOINT_1")).thenReturn(Mono.empty());
369-
when(connectionMock.releaseSavepoint("SAVEPOINT_1")).thenReturn(Mono.empty());
371+
when(connectionMock.createSavepoint(anyString())).thenReturn(Mono.empty());
372+
when(connectionMock.rollbackTransactionToSavepoint(anyString())).thenReturn(Mono.empty());
373+
when(connectionMock.releaseSavepoint(anyString())).thenReturn(Mono.empty());
370374
when(connectionMock.commitTransaction()).thenReturn(Mono.empty());
371375

372376
DefaultTransactionDefinition definition = new DefaultTransactionDefinition();
373377
definition.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
374378

375379
TransactionalOperator operator = TransactionalOperator.create(tm, definition);
376-
operator.execute(tx1 -> {
377-
assertThat(tx1.isNewTransaction()).isTrue();
378-
definition.setPropagationBehavior(TransactionDefinition.PROPAGATION_NESTED);
379-
return operator.execute(tx2 -> {
380-
assertThat(tx2.isNewTransaction()).isTrue();
381-
return Mono.empty();
382-
});
383-
}).as(StepVerifier::create).verifyComplete();
384-
385-
verify(connectionMock).createSavepoint("SAVEPOINT_1");
386-
verify(connectionMock).releaseSavepoint("SAVEPOINT_1");
387-
verify(connectionMock).commitTransaction();
388-
verify(connectionMock).close();
389-
}
390-
391-
@Test
392-
void testPropagationNestedWithExistingTransactionAndRollback() {
393-
when(connectionMock.createSavepoint("SAVEPOINT_1")).thenReturn(Mono.empty());
394-
when(connectionMock.rollbackTransactionToSavepoint("SAVEPOINT_1")).thenReturn(Mono.empty());
395-
when(connectionMock.commitTransaction()).thenReturn(Mono.empty());
396-
397-
DefaultTransactionDefinition definition = new DefaultTransactionDefinition();
398-
definition.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
399-
400-
TransactionalOperator operator = TransactionalOperator.create(tm, definition);
401-
operator.execute(tx1 -> {
402-
assertThat(tx1.isNewTransaction()).isTrue();
380+
operator.execute(tx -> {
381+
assertThat(tx.isNewTransaction()).isTrue();
403382
definition.setPropagationBehavior(TransactionDefinition.PROPAGATION_NESTED);
404-
return operator.execute(tx2 -> {
405-
assertThat(tx2.isNewTransaction()).isTrue();
406-
tx2.setRollbackOnly();
407-
return Mono.empty();
408-
});
383+
return Flux.concat(
384+
TransactionalOperator.create(tm, definition).execute(ntx1 -> {
385+
assertThat(ntx1.isNewTransaction()).as("ntx1.isNewTransaction()").isTrue();
386+
assertThat(ntx1.isRollbackOnly()).as("ntx1.isRollbackOnly()").isFalse();
387+
return Mono.empty();
388+
}),
389+
TransactionalOperator.create(tm, definition).execute(ntx2 -> {
390+
assertThat(ntx2.isNewTransaction()).as("ntx2.isNewTransaction()").isTrue();
391+
assertThat(ntx2.isRollbackOnly()).as("ntx2.isRollbackOnly()").isFalse();
392+
ntx2.setRollbackOnly();
393+
assertThat(ntx2.isRollbackOnly()).isTrue();
394+
return Mono.empty();
395+
}),
396+
TransactionalOperator.create(tm, definition).execute(ntx3 -> {
397+
assertThat(ntx3.isNewTransaction()).as("ntx3.isNewTransaction()").isTrue();
398+
assertThat(ntx3.isRollbackOnly()).as("ntx3.isRollbackOnly()").isFalse();
399+
return Mono.empty();
400+
}),
401+
TransactionalOperator.create(tm, definition).execute(ntx4 -> {
402+
assertThat(ntx4.isNewTransaction()).as("ntx4.isNewTransaction()").isTrue();
403+
assertThat(ntx4.isRollbackOnly()).as("ntx4.isRollbackOnly()").isFalse();
404+
ntx4.setRollbackOnly();
405+
assertThat(ntx4.isRollbackOnly()).isTrue();
406+
return Flux.concat(
407+
TransactionalOperator.create(tm, definition).execute(ntx4n1 -> {
408+
assertThat(ntx4n1.isNewTransaction()).as("ntx4n1.isNewTransaction()").isTrue();
409+
assertThat(ntx4n1.isRollbackOnly()).as("ntx4n1.isRollbackOnly()").isFalse();
410+
return Mono.empty();
411+
}),
412+
TransactionalOperator.create(tm, definition).execute(ntx4n2 -> {
413+
assertThat(ntx4n2.isNewTransaction()).as("ntx4n2.isNewTransaction()").isTrue();
414+
assertThat(ntx4n2.isRollbackOnly()).as("ntx4n2.isRollbackOnly()").isFalse();
415+
ntx4n2.setRollbackOnly();
416+
assertThat(ntx4n2.isRollbackOnly()).isTrue();
417+
return Mono.empty();
418+
})
419+
);
420+
}),
421+
TransactionalOperator.create(tm, definition).execute(ntx5 -> {
422+
assertThat(ntx5.isNewTransaction()).as("ntx5.isNewTransaction()").isTrue();
423+
assertThat(ntx5.isRollbackOnly()).as("ntx5.isRollbackOnly()").isFalse();
424+
ntx5.setRollbackOnly();
425+
assertThat(ntx5.isRollbackOnly()).isTrue();
426+
return Flux.concat(
427+
TransactionalOperator.create(tm, definition).execute(ntx5n1 -> {
428+
assertThat(ntx5n1.isNewTransaction()).as("ntx5n1.isNewTransaction()").isTrue();
429+
assertThat(ntx5n1.isRollbackOnly()).as("ntx5n1.isRollbackOnly()").isFalse();
430+
return Mono.empty();
431+
}),
432+
TransactionalOperator.create(tm, definition).execute(ntx5n2 -> {
433+
assertThat(ntx5n2.isNewTransaction()).as("ntx5n2.isNewTransaction()").isTrue();
434+
assertThat(ntx5n2.isRollbackOnly()).as("ntx5n2.isRollbackOnly()").isFalse();
435+
ntx5n2.setRollbackOnly();
436+
assertThat(ntx5n2.isRollbackOnly()).isTrue();
437+
return Mono.empty();
438+
})
439+
);
440+
})
441+
);
409442
}).as(StepVerifier::create).verifyComplete();
410443

411-
verify(connectionMock).createSavepoint("SAVEPOINT_1");
412-
verify(connectionMock).rollbackTransactionToSavepoint("SAVEPOINT_1");
413-
verify(connectionMock).commitTransaction();
414-
verify(connectionMock).close();
444+
InOrder inOrder = inOrder(connectionMock);
445+
// ntx1
446+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_1");
447+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_1");
448+
// ntx2
449+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_2");
450+
inOrder.verify(connectionMock).rollbackTransactionToSavepoint("SAVEPOINT_2");
451+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_2");
452+
// ntx3
453+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_3");
454+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_3");
455+
// ntx4
456+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_4");
457+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_5");
458+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_5");
459+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_6");
460+
inOrder.verify(connectionMock).rollbackTransactionToSavepoint("SAVEPOINT_6");
461+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_6");
462+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_4");
463+
// ntx5
464+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_7");
465+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_8");
466+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_8");
467+
inOrder.verify(connectionMock).createSavepoint("SAVEPOINT_9");
468+
inOrder.verify(connectionMock).rollbackTransactionToSavepoint("SAVEPOINT_9");
469+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_9");
470+
inOrder.verify(connectionMock).rollbackTransactionToSavepoint("SAVEPOINT_7");
471+
inOrder.verify(connectionMock).releaseSavepoint("SAVEPOINT_7");
472+
// tx
473+
inOrder.verify(connectionMock).commitTransaction();
474+
inOrder.verify(connectionMock).close();
415475
}
416476

417477
@Test
@@ -452,7 +512,9 @@ void testPropagationSupportsAndNestedWithRollback() {
452512
TransactionalOperator inner = TransactionalOperator.create(tm, innerDef);
453513
return inner.execute(tx2 -> {
454514
assertThat(tx2.isNewTransaction()).isTrue();
515+
assertThat(tx2.isRollbackOnly()).isFalse();
455516
tx2.setRollbackOnly();
517+
assertThat(tx2.isRollbackOnly()).isTrue();
456518
return Mono.empty();
457519
});
458520
}).as(StepVerifier::create).verifyComplete();
@@ -499,7 +561,9 @@ void testPropagationSupportsAndRequiresNewWithRollback() {
499561
TransactionalOperator inner = TransactionalOperator.create(tm, innerDef);
500562
return inner.execute(tx2 -> {
501563
assertThat(tx2.isNewTransaction()).isTrue();
564+
assertThat(tx2.isRollbackOnly()).isFalse();
502565
tx2.setRollbackOnly();
566+
assertThat(tx2.isRollbackOnly()).isTrue();
503567
return Mono.empty();
504568
});
505569
}).as(StepVerifier::create).verifyComplete();

0 commit comments

Comments
 (0)