DATAREDIS-684 - Release Jedis cluster node connections with close().

We now release Jedis cluster node connections with Jedis.close() to the pool instead of Pool.returnResource(…). The close() method itself checks whether the connection was broken and if so, the connection gets destroyed. Destroying broken connections prevents the pool from supplying broken connections on borrow when testOnBorrow is disabled.

The only case where we return broken resources ourselves to the Pool is when we discover a broken connection ourselves: If we run into a NullPointerException or RedisConnectionFailureException, then we consider a connection is broken.

Original Pull Request: #271

src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java

@@ -4167,7 +4167,7 @@ public class JedisClusterConnection implements RedisClusterConnection {
 		 */
 		@Override
 		public void returnResourceForSpecificNode(RedisClusterNode node, Object client) {
-			getResourcePoolForSpecificNode(node).returnResource((Jedis) client);
+			((Jedis) client).close();
 		}
 	}
 
@@ -4224,7 +4224,7 @@ public class JedisClusterConnection implements RedisClusterConnection {
 					errors.put(entry.getKey(), ex);
 				} finally {
 					if (jedis != null) {
-						entry.getValue().returnResource(jedis);
+						jedis.close();
 					}
 				}
 			}

src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java

@@ -284,29 +284,27 @@ public class JedisConnection extends AbstractRedisConnection {
 	}
 
 	public void close() throws DataAccessException {
+
 		super.close();
+
 		// return the connection to the pool
 		if (pool != null) {
-			if (!broken) {
+
+			if (broken) {
+				pool.returnBrokenResource(jedis);
+			} else {
+
 				// reset the connection
 				try {
 					if (dbIndex > 0) {
 						jedis.select(0);
 					}
-					pool.returnResource(jedis);
 					return;
 				} catch (Exception ex) {
-					DataAccessException dae = convertJedisAccessException(ex);
-					if (broken) {
-						pool.returnBrokenResource(jedis);
-					} else {
-						pool.returnResource(jedis);
-					}
-					throw dae;
+					throw convertJedisAccessException(ex);
+				} finally {
+					jedis.close();
 				}
-			} else {
-				pool.returnBrokenResource(jedis);
-				return;
 			}
 		}
 		// else close the connection normally (doing the try/catch dance)
@@ -314,13 +312,13 @@ public class JedisConnection extends AbstractRedisConnection {
 		if (isQueueing()) {
 			try {
 				client.quit();
-			} catch (Exception ex) {
-				exc = ex;
+			} catch (Exception o_O) {
+				// ignore exception
 			}
 			try {
 				client.disconnect();
-			} catch (Exception ex) {
-				exc = ex;
+			} catch (Exception o_O) {
+				// ignore exception
 			}
 			return;
 		}

src/test/java/org/springframework/data/redis/connection/ClusterSlotHashUtilsTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2015-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,11 @@ package org.springframework.data.redis.connection;
 
 import static org.junit.Assert.*;
 
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.JedisPool;
+
 import java.io.IOException;
 import java.util.Collections;
 import java.util.Random;
@@ -26,13 +31,9 @@ import org.junit.Test;
 import org.springframework.data.redis.test.util.RedisClusterRule;
 import org.springframework.util.StringUtils;
 
-import redis.clients.jedis.HostAndPort;
-import redis.clients.jedis.Jedis;
-import redis.clients.jedis.JedisCluster;
-import redis.clients.jedis.JedisPool;
-
 /**
  * @author Christoph Strobl
+ * @author Mark Paluch
  */
 public class ClusterSlotHashUtilsTests {
 
@@ -58,7 +59,7 @@ public class ClusterSlotHashUtilsTests {
 						serverSlot.intValue(), slot);
 
 			}
-			pool.returnResource(jedis);
+			jedis.close();
 		} finally {
 			if (cluster != null) {
 				cluster.close();
@@ -100,7 +101,7 @@ public class ClusterSlotHashUtilsTests {
 						serverSlot1.intValue(), slot1);
 
 			}
-			pool.returnResource(jedis);
+			jedis.close();
 		} finally {
 			if (cluster != null) {
 				cluster.close();
@@ -110,7 +111,7 @@ public class ClusterSlotHashUtilsTests {
 
 	/**
 	 * Generate random string using ascii chars {@code ' ' (space)} to {@code 'z'}. Explicitly skipping { and }.
-	 * 
+	 *
 	 * @return
 	 */
 	private String randomString() {

src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java

@@ -150,6 +150,7 @@ public class JedisClusterConnectionUnitTests {
 
 		verify(con2Mock, times(1)).clusterReplicate(CLUSTER_NODE_1.getId());
 		verify(con1Mock, times(1)).clusterNodes();
+		verify(con1Mock, times(1)).close();
 		verifyZeroInteractions(con1Mock);
 	}
 
@@ -310,7 +311,9 @@ public class JedisClusterConnectionUnitTests {
 		connection.time(CLUSTER_NODE_2);
 
 		verify(con2Mock, times(1)).time();
+		verify(con2Mock, times(1)).close();
 		verify(con1Mock, times(1)).clusterNodes();
+		verify(con1Mock, times(1)).close();
 		verifyZeroInteractions(con1Mock, con3Mock);
 	}
 
@@ -330,6 +333,7 @@ public class JedisClusterConnectionUnitTests {
 		connection.resetConfigStats(CLUSTER_NODE_2);
 
 		verify(con2Mock, times(1)).configResetStat();
+		verify(con2Mock, times(1)).close();
 		verify(con1Mock, never()).configResetStat();
 		verify(con3Mock, never()).configResetStat();
 	}