DATAREDIS-891 - Revise RedisConnection transaction/closure resource binding.

We now correctly consider the transactional state along with the requested bindings scope when binding RedisConnection for reuse.

Previously, enabling transaction support on RedisTemplate registered the connection unconditionally with TransactionSynchronizationManager expecting a TransactionManager to close/unbind the connection upon transaction cleanup. This behavior could easily lead to lingering connections when a transactional RedisTemplate was used outside of a managed transaction.

We now distinguish between closure-scope binding and transaction binding.

Closure-scoped bindings (RedisTemplate.execute(SessionCallback)) do not interfere with transactional resources if there's no transactionally bound RedisConnection.

Closure-scope however reuses a transactionally bound connection.

Transaction-scoped binding binds only a connection to the transaction if there's an active transaction. Without an ongoing transaction, RedisTemplate does no longer bind connections to TransactionSynchronizationManager.

Original Pull Request: #573

src/test/java/org/springframework/data/redis/core/ConnectionSplittingInterceptorUnitTests.java

@@ -69,7 +69,7 @@ class ConnectionSplittingInterceptorUnitTests {
 	@Test // DATAREDIS-73
 	void interceptorShouldRequestFreshConnectionForReadonlyCommand() throws Throwable {
 
-		interceptor.intercept(boundConnectionMock, READONLY_METHOD, new Object[] { new byte[] {} }, null);
+		interceptor.intercept(boundConnectionMock, READONLY_METHOD, new Object[] { new byte[] {} });
 		verify(connectionFactoryMock, times(1)).getConnection();
 		verifyZeroInteractions(boundConnectionMock);
 	}
@@ -77,7 +77,7 @@ class ConnectionSplittingInterceptorUnitTests {
 	@Test // DATAREDIS-73
 	void interceptorShouldUseBoundConnectionForWriteOperations() throws Throwable {
 
-		interceptor.intercept(boundConnectionMock, WRITE_METHOD, new Object[] { new byte[] {}, 0L }, null);
+		interceptor.intercept(boundConnectionMock, WRITE_METHOD, new Object[] { new byte[] {}, 0L });
 		verify(boundConnectionMock, times(1)).expire(any(byte[].class), anyLong());
 		verifyZeroInteractions(connectionFactoryMock);
 	}
@@ -90,7 +90,7 @@ class ConnectionSplittingInterceptorUnitTests {
 				InvalidDataAccessApiUsageException.class);
 
 		Assertions.assertThatExceptionOfType(InvalidDataAccessApiUsageException.class).isThrownBy(
-				() -> interceptor.intercept(boundConnectionMock, READONLY_METHOD, new Object[] { new byte[] {} }, null));
+				() -> interceptor.intercept(boundConnectionMock, READONLY_METHOD, new Object[] { new byte[] {} }));
 	}
 
 }

src/test/java/org/springframework/data/redis/core/RedisConnectionUtilsUnitTests.java

@@ -15,12 +15,21 @@
  */
 package org.springframework.data.redis.core;
 
+import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
 
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
+import org.springframework.transaction.TransactionDefinition;
+import org.springframework.transaction.TransactionException;
+import org.springframework.transaction.support.AbstractPlatformTransactionManager;
+import org.springframework.transaction.support.DefaultTransactionStatus;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
+import org.springframework.transaction.support.TransactionTemplate;
 
 /**
  * Unit tests for {@link RedisConnectionUtils}.
@@ -29,16 +38,188 @@ import org.springframework.data.redis.connection.RedisConnectionFactory;
  */
 class RedisConnectionUtilsUnitTests {
 
+	RedisConnection connectionMock1 = mock(RedisConnection.class);
+	RedisConnection connectionMock2 = mock(RedisConnection.class);
+	RedisConnectionFactory factoryMock = mock(RedisConnectionFactory.class);
+
+	@BeforeEach
+	void setUp() {
+
+		// cleanup to avoid lingering resources
+		TransactionSynchronizationManager.clear();
+
+		if (TransactionSynchronizationManager.isSynchronizationActive()) {
+			TransactionSynchronizationManager.clearSynchronization();
+		}
+
+		when(factoryMock.getConnection()).thenReturn(connectionMock1, connectionMock2);
+	}
+
 	@Test // DATAREDIS-1104
 	void shouldSilentlyCloseRedisConnection() {
 
-		RedisConnection connectionMock = mock(RedisConnection.class);
-		RedisConnectionFactory factoryMock = mock(RedisConnectionFactory.class);
+		Mockito.reset(connectionMock1);
+		doThrow(new IllegalStateException()).when(connectionMock1).close();
+		RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock, false);
 
-		doThrow(new IllegalStateException()).when(connectionMock).close();
+		verify(connectionMock1).close();
+	}
 
-		RedisConnectionUtils.releaseConnection(connectionMock, factoryMock, false);
+	@Test // DATAREDIS-891
+	void bindConnectionShouldBindConnectionToClosureScope() {
 
-		verify(connectionMock).close();
+		assertThat(RedisConnectionUtils.bindConnection(factoryMock)).isSameAs(connectionMock1);
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+
+		assertThat(RedisConnectionUtils.getConnection(factoryMock)).isSameAs(connectionMock1);
+
+		RedisConnectionUtils.unbindConnection(factoryMock);
+		verifyNoInteractions(connectionMock1);
+
+		RedisConnectionUtils.unbindConnection(factoryMock);
+		verify(connectionMock1).close();
+
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	@Test // DATAREDIS-891
+	void getConnectionShouldBindConnectionToTransactionScopeWithReadOnlyTransaction() {
+
+		TransactionTemplate template = new TransactionTemplate(new DummyTransactionManager());
+		template.setReadOnly(true);
+
+		template.executeWithoutResult(status -> {
+
+			assertThat(RedisConnectionUtils.getConnection(factoryMock, true)).isSameAs(connectionMock1);
+			assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+			assertThat(RedisConnectionUtils.getConnection(factoryMock)).isSameAs(connectionMock1);
+
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+
+			verifyNoInteractions(connectionMock1);
+		});
+
+		verify(connectionMock1).close();
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	@Test // DATAREDIS-891
+	void bindConnectionShouldBindConnectionToOngoingTransactionScope() {
+
+		TransactionTemplate template = new TransactionTemplate(new DummyTransactionManager());
+
+		template.executeWithoutResult(status -> {
+
+			assertThat(RedisConnectionUtils.bindConnection(factoryMock, true)).isNotNull();
+			assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+			assertThat(RedisConnectionUtils.getConnection(factoryMock)).isNotNull();
+
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+
+			verify(connectionMock1).multi();
+			verifyNoMoreInteractions(connectionMock1);
+		});
+
+		verify(connectionMock1).close();
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	@Test // DATAREDIS-891
+	void bindConnectionShouldNotBindConnectionToTransactionWithoutTransaction() {
+
+		assertThat(RedisConnectionUtils.bindConnection(factoryMock, true)).isNotNull();
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+		assertThat(RedisConnectionUtils.getConnection(factoryMock)).isNotNull();
+
+		RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+		RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+
+		verify(connectionMock1).close();
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	@Test // DATAREDIS-891
+	void getConnectionShouldBindConnectionToTransactionScopeWithReadWriteTransaction() {
+
+		TransactionTemplate template = new TransactionTemplate(new DummyTransactionManager());
+
+		template.executeWithoutResult(status -> {
+
+			assertThat(RedisConnectionUtils.getConnection(factoryMock, true)).isNotNull();
+			assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+
+			verify(connectionMock1).multi();
+			verify(factoryMock, times(1)).getConnection();
+		});
+
+		verify(connectionMock1).close();
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	@Test // DATAREDIS-891
+	void getConnectionShouldNotBindConnectionToTransaction() {
+
+		TransactionTemplate template = new TransactionTemplate(new DummyTransactionManager());
+
+		template.executeWithoutResult(status -> {
+
+			assertThat(RedisConnectionUtils.getConnection(factoryMock)).isSameAs(connectionMock1);
+			assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+
+			RedisConnectionUtils.releaseConnection(connectionMock1, factoryMock);
+
+			verify(factoryMock).getConnection();
+			verify(connectionMock1).close();
+		});
+
+		verifyNoMoreInteractions(factoryMock);
+		verifyNoMoreInteractions(connectionMock1);
+	}
+
+	@Test // DATAREDIS-891
+	void bindConnectionShouldNotBindConnectionToTransaction() {
+
+		TransactionTemplate template = new TransactionTemplate(new DummyTransactionManager());
+
+		template.executeWithoutResult(status -> {
+
+			assertThat(RedisConnectionUtils.bindConnection(factoryMock)).isSameAs(connectionMock1);
+			assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isTrue();
+
+			RedisConnectionUtils.unbindConnection(factoryMock);
+
+			verify(connectionMock1).close();
+			verify(factoryMock, times(1)).getConnection();
+		});
+
+		verifyNoMoreInteractions(factoryMock);
+		assertThat(TransactionSynchronizationManager.hasResource(factoryMock)).isFalse();
+	}
+
+	static class DummyTransactionManager extends AbstractPlatformTransactionManager {
+
+		@Override
+		protected Object doGetTransaction() throws TransactionException {
+			return new Object();
+		}
+
+		@Override
+		protected void doBegin(Object transaction, TransactionDefinition definition) throws TransactionException {
+
+		}
+
+		@Override
+		protected void doCommit(DefaultTransactionStatus status) throws TransactionException {
+
+		}
+
+		@Override
+		protected void doRollback(DefaultTransactionStatus status) throws TransactionException {
+
+		}
 	}
 }

src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java

@@ -122,8 +122,6 @@ class RedisTemplateUnitTests {
 	@Test // DATAREDIS-988
 	void executeSessionInTransactionShouldReuseConnection() {
 
-		TransactionSynchronizationManager.setCurrentTransactionReadOnly(true);
-
 		template.execute(new SessionCallback<Object>() {
 			@Override
 			public <K, V> Object execute(RedisOperations<K, V> operations) throws DataAccessException {
@@ -142,12 +140,11 @@ class RedisTemplateUnitTests {
 	void transactionAwareTemplateShouldReleaseConnection() {
 
 		template.setEnableTransactionSupport(true);
-		TransactionSynchronizationManager.setCurrentTransactionReadOnly(true);
-
-		template.execute(new SessionCallback<Object>() {
 
+		template.execute(new RedisCallback<Object>() {
+			@Nullable
 			@Override
-			public <K, V> Object execute(RedisOperations<K, V> operations) throws DataAccessException {
+			public Object doInRedis(RedisConnection connection) throws DataAccessException {
 
 				template.multi();
 				template.multi();
@@ -155,8 +152,8 @@ class RedisTemplateUnitTests {
 			}
 		});
 
-		verify(connectionFactoryMock, times(2)).getConnection();
-		verify(redisConnectionMock, times(2)).close();
+		verify(connectionFactoryMock, times(3)).getConnection();
+		verify(redisConnectionMock, times(3)).close();
 	}
 
 	private static class SomeArbitrarySerializableObject implements Serializable {