Skip to content

Commit 9751987

Browse files
jhoellerjhoeller
committed
Avoid unnecessary auto-commit check for proper transaction begin
Closes gh-30508
1 parent d290625 commit 9751987

File tree

3 files changed

+18
-97
lines changed

3 files changed

+18
-97
lines changed

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public abstract class ConnectionFactoryUtils {
8787
*/
8888
public static Mono<Connection> getConnection(ConnectionFactory connectionFactory) {
8989
return doGetConnection(connectionFactory)
90-
.onErrorMap(e -> new DataAccessResourceFailureException("Failed to obtain R2DBC Connection", e));
90+
.onErrorMap(ex -> new DataAccessResourceFailureException("Failed to obtain R2DBC Connection", ex));
9191
}
9292

9393
/**
@@ -133,10 +133,10 @@ public static Mono<Connection> doGetConnection(ConnectionFactory connectionFacto
133133
synchronizationManager.bindResource(connectionFactory, holderToUse);
134134
}
135135
}) // Unexpected exception from external delegation call -> close Connection and rethrow.
136-
.onErrorResume(e -> releaseConnection(connection, connectionFactory).then(Mono.error(e))));
136+
.onErrorResume(ex -> releaseConnection(connection, connectionFactory).then(Mono.error(ex))));
137137
}
138138
return con;
139-
}).onErrorResume(NoTransactionException.class, e -> Mono.from(connectionFactory.create()));
139+
}).onErrorResume(NoTransactionException.class, ex -> Mono.from(connectionFactory.create()));
140140
}
141141

142142
/**
@@ -161,7 +161,7 @@ private static Mono<Connection> fetchConnection(ConnectionFactory connectionFact
161161
*/
162162
public static Mono<Void> releaseConnection(Connection con, ConnectionFactory connectionFactory) {
163163
return doReleaseConnection(con, connectionFactory)
164-
.onErrorMap(e -> new DataAccessResourceFailureException("Failed to close R2DBC Connection", e));
164+
.onErrorMap(ex -> new DataAccessResourceFailureException("Failed to close R2DBC Connection", ex));
165165
}
166166

167167
/**
@@ -181,7 +181,7 @@ public static Mono<Void> doReleaseConnection(Connection connection, ConnectionFa
181181
conHolder.released();
182182
}
183183
return Mono.from(connection.close());
184-
}).onErrorResume(NoTransactionException.class, e -> Mono.from(connection.close()));
184+
}).onErrorResume(NoTransactionException.class, ex -> Mono.from(connection.close()));
185185
}
186186

187187
/**

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

Lines changed: 11 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,7 @@ protected Mono<Void> doBegin(TransactionSynchronizationManager synchronizationMa
210210
connectionMono = Mono.just(txObject.getConnectionHolder().getConnection());
211211
}
212212

213-
return connectionMono.flatMap(con -> switchAutoCommitIfNecessary(con, transaction)
214-
.then(Mono.from(doBegin(definition, con)))
213+
return connectionMono.flatMap(con -> Mono.from(doBegin(definition, con))
215214
.then(prepareTransactionalConnection(con, definition))
216215
.doOnSuccess(v -> {
217216
txObject.getConnectionHolder().setTransactionActive(true);
@@ -223,18 +222,15 @@ protected Mono<Void> doBegin(TransactionSynchronizationManager synchronizationMa
223222
if (txObject.isNewConnectionHolder()) {
224223
synchronizationManager.bindResource(obtainConnectionFactory(), txObject.getConnectionHolder());
225224
}
226-
}).thenReturn(con).onErrorResume(e -> {
225+
}).thenReturn(con).onErrorResume(ex -> {
227226
if (txObject.isNewConnectionHolder()) {
228227
return ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory())
229228
.doOnTerminate(() -> txObject.setConnectionHolder(null, false))
230-
.then(Mono.error(e));
229+
.then(Mono.error(ex));
231230
}
232-
return Mono.error(e);
233-
})).onErrorResume(e -> {
234-
CannotCreateTransactionException ex = new CannotCreateTransactionException(
235-
"Could not open R2DBC Connection for transaction", e);
236231
return Mono.error(ex);
237-
});
232+
})).onErrorResume(ex -> Mono.error(new CannotCreateTransactionException(
233+
"Could not open R2DBC Connection for transaction", ex)));
238234
}).then();
239235
}
240236

@@ -356,20 +352,18 @@ protected Mono<Void> doCleanupAfterCompletion(TransactionSynchronizationManager
356352

357353
Mono<Void> afterCleanup = Mono.empty();
358354

359-
if (txObject.isMustRestoreAutoCommit()) {
360-
Mono<Void> restoreAutoCommitStep = safeCleanupStep(
361-
"doCleanupAfterCompletion when restoring autocommit", Mono.from(con.setAutoCommit(true)));
362-
afterCleanup = afterCleanup.then(restoreAutoCommitStep);
363-
}
364-
365355
Mono<Void> releaseConnectionStep = Mono.defer(() -> {
366356
try {
367357
if (txObject.isNewConnectionHolder()) {
368358
if (logger.isDebugEnabled()) {
369359
logger.debug("Releasing R2DBC Connection [" + con + "] after transaction");
370360
}
371-
return safeCleanupStep("doCleanupAfterCompletion when releasing R2DBC Connection",
372-
ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory()));
361+
Mono<Void> releaseMono = ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory());
362+
if (logger.isDebugEnabled()) {
363+
releaseMono = releaseMono.doOnError(
364+
ex -> logger.debug(String.format("Error ignored during cleanup: %s", ex)));
365+
}
366+
return releaseMono.onErrorComplete();
373367
}
374368
}
375369
finally {
@@ -381,35 +375,6 @@ protected Mono<Void> doCleanupAfterCompletion(TransactionSynchronizationManager
381375
});
382376
}
383377

384-
private Mono<Void> safeCleanupStep(String stepDescription, Mono<Void> stepMono) {
385-
if (!logger.isDebugEnabled()) {
386-
return stepMono.onErrorComplete();
387-
}
388-
else {
389-
return stepMono.doOnError(e ->
390-
logger.debug(String.format("Error ignored during %s: %s", stepDescription, e)))
391-
.onErrorComplete();
392-
}
393-
}
394-
395-
private Mono<Void> switchAutoCommitIfNecessary(Connection con, Object transaction) {
396-
ConnectionFactoryTransactionObject txObject = (ConnectionFactoryTransactionObject) transaction;
397-
Mono<Void> prepare = Mono.empty();
398-
399-
// Switch to manual commit if necessary. This is very expensive in some R2DBC drivers,
400-
// so we don't want to do it unnecessarily (for example if we've explicitly
401-
// configured the connection pool to set it already).
402-
if (con.isAutoCommit()) {
403-
txObject.setMustRestoreAutoCommit(true);
404-
if (logger.isDebugEnabled()) {
405-
logger.debug("Switching R2DBC Connection [" + con + "] to manual commit");
406-
}
407-
prepare = prepare.then(Mono.from(con.setAutoCommit(false)));
408-
}
409-
410-
return prepare;
411-
}
412-
413378
/**
414379
* Prepare the transactional {@link Connection} right after transaction begin.
415380
* <p>The default implementation executes a "SET TRANSACTION READ ONLY" statement if the
@@ -531,8 +496,6 @@ private static class ConnectionFactoryTransactionObject {
531496

532497
private boolean newConnectionHolder;
533498

534-
private boolean mustRestoreAutoCommit;
535-
536499
void setConnectionHolder(@Nullable ConnectionHolder connectionHolder, boolean newConnectionHolder) {
537500
setConnectionHolder(connectionHolder);
538501
this.newConnectionHolder = newConnectionHolder;
@@ -558,14 +521,6 @@ public ConnectionHolder getConnectionHolder() {
558521
public boolean hasConnectionHolder() {
559522
return (this.connectionHolder != null);
560523
}
561-
562-
public void setMustRestoreAutoCommit(boolean mustRestoreAutoCommit) {
563-
this.mustRestoreAutoCommit = mustRestoreAutoCommit;
564-
}
565-
566-
public boolean isMustRestoreAutoCommit() {
567-
return this.mustRestoreAutoCommit;
568-
}
569524
}
570525

571526
}

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

Lines changed: 2 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import io.r2dbc.spi.ConnectionFactory;
2424
import io.r2dbc.spi.IsolationLevel;
2525
import io.r2dbc.spi.R2dbcBadGrammarException;
26-
import io.r2dbc.spi.R2dbcTimeoutException;
2726
import io.r2dbc.spi.Statement;
2827
import org.junit.jupiter.api.BeforeEach;
2928
import org.junit.jupiter.api.Test;
@@ -56,6 +55,7 @@
5655
* Unit tests for {@link R2dbcTransactionManager}.
5756
*
5857
* @author Mark Paluch
58+
* @author Juergen Hoeller
5959
*/
6060
class R2dbcTransactionManagerUnitTests {
6161

@@ -67,7 +67,7 @@ class R2dbcTransactionManagerUnitTests {
6767

6868

6969
@BeforeEach
70-
@SuppressWarnings({ "unchecked", "rawtypes" })
70+
@SuppressWarnings({"unchecked", "rawtypes"})
7171
void before() {
7272
when(connectionFactoryMock.create()).thenReturn((Mono) Mono.just(connectionMock));
7373
when(connectionMock.beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class))).thenReturn(Mono.empty());
@@ -96,7 +96,6 @@ void testSimpleTransaction() {
9696
.verifyComplete();
9797

9898
assertThat(commits).hasValue(1);
99-
verify(connectionMock).isAutoCommit();
10099
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
101100
verify(connectionMock).commitTransaction();
102101
verify(connectionMock).close();
@@ -185,7 +184,6 @@ void doesNotSetIsolationLevelIfMatch() {
185184

186185
@Test
187186
void doesNotSetAutoCommitDisabled() {
188-
when(connectionMock.isAutoCommit()).thenReturn(false);
189187
when(connectionMock.commitTransaction()).thenReturn(Mono.empty());
190188

191189
DefaultTransactionDefinition definition = new DefaultTransactionDefinition();
@@ -203,29 +201,6 @@ void doesNotSetAutoCommitDisabled() {
203201
verify(connectionMock).commitTransaction();
204202
}
205203

206-
@Test
207-
void restoresAutoCommit() {
208-
when(connectionMock.isAutoCommit()).thenReturn(true);
209-
when(connectionMock.setAutoCommit(anyBoolean())).thenReturn(Mono.empty());
210-
when(connectionMock.commitTransaction()).thenReturn(Mono.empty());
211-
212-
DefaultTransactionDefinition definition = new DefaultTransactionDefinition();
213-
214-
TransactionalOperator operator = TransactionalOperator.create(tm, definition);
215-
216-
ConnectionFactoryUtils.getConnection(connectionFactoryMock).as(
217-
operator::transactional)
218-
.as(StepVerifier::create)
219-
.expectNextCount(1)
220-
.verifyComplete();
221-
222-
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
223-
verify(connectionMock).setAutoCommit(false);
224-
verify(connectionMock).setAutoCommit(true);
225-
verify(connectionMock).commitTransaction();
226-
verify(connectionMock).close();
227-
}
228-
229204
@Test
230205
void appliesReadOnly() {
231206
when(connectionMock.commitTransaction()).thenReturn(Mono.empty());
@@ -246,7 +221,6 @@ void appliesReadOnly() {
246221
.expectNextCount(1)
247222
.verifyComplete();
248223

249-
verify(connectionMock).isAutoCommit();
250224
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
251225
verify(connectionMock).createStatement("SET TRANSACTION READ ONLY");
252226
verify(connectionMock).commitTransaction();
@@ -268,7 +242,6 @@ void testCommitFails() {
268242
.as(StepVerifier::create)
269243
.verifyError(BadSqlGrammarException.class);
270244

271-
verify(connectionMock).isAutoCommit();
272245
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
273246
verify(connectionMock).createStatement("foo");
274247
verify(connectionMock).commitTransaction();
@@ -299,7 +272,6 @@ void testRollback() {
299272

300273
assertThat(commits).hasValue(0);
301274
assertThat(rollbacks).hasValue(1);
302-
verify(connectionMock).isAutoCommit();
303275
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
304276
verify(connectionMock).rollbackTransaction();
305277
verify(connectionMock).close();
@@ -322,7 +294,6 @@ void testRollbackFails() {
322294
}).as(StepVerifier::create)
323295
.verifyError(BadSqlGrammarException.class);
324296

325-
verify(connectionMock).isAutoCommit();
326297
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
327298
verify(connectionMock).createStatement("foo");
328299
verify(connectionMock, never()).commitTransaction();
@@ -338,10 +309,7 @@ void testConnectionReleasedWhenRollbackFails() {
338309

339310
TransactionalOperator operator = TransactionalOperator.create(tm);
340311

341-
when(connectionMock.isAutoCommit()).thenReturn(true);
342-
when(connectionMock.setAutoCommit(true)).thenReturn(Mono.defer(() -> Mono.error(new R2dbcTimeoutException("SET AUTOCOMMIT = 1 timed out"))));
343312
when(connectionMock.setTransactionIsolationLevel(any())).thenReturn(Mono.empty());
344-
when(connectionMock.setAutoCommit(false)).thenReturn(Mono.empty());
345313

346314
operator.execute(reactiveTransaction -> ConnectionFactoryUtils.getConnection(connectionFactoryMock)
347315
.doOnNext(connection -> {
@@ -352,7 +320,6 @@ void testConnectionReleasedWhenRollbackFails() {
352320
.hasCause(new R2dbcBadGrammarException("Rollback should fail"))
353321
);
354322

355-
verify(connectionMock).isAutoCommit();
356323
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
357324
verify(connectionMock, never()).commitTransaction();
358325
verify(connectionMock).rollbackTransaction();
@@ -380,7 +347,6 @@ void testTransactionSetRollbackOnly() {
380347
}).as(StepVerifier::create)
381348
.verifyComplete();
382349

383-
verify(connectionMock).isAutoCommit();
384350
verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class));
385351
verify(connectionMock).rollbackTransaction();
386352
verify(connectionMock).close();

0 commit comments

Comments
 (0)