From 453af21e0df74479b9a0781f8b27f8120ab241bc Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 31 Aug 2016 12:22:58 +0200 Subject: [PATCH 1/3] DATAREDIS-531 - ScanCursor holds reference to closed/released Jedis connection. Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 2486f880c8..ad2140fcf9 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 1.8.0.BUILD-SNAPSHOT + 1.8.0.DATAREDIS-531-SNAPSHOT Spring Data Redis From d2ca312b7f94023dac63d41dd0b44eff4e98270d Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 31 Aug 2016 12:28:04 +0200 Subject: [PATCH 2/3] DATAREDIS-531 - Scan commands should use a dedicated bound connection. SCAN, SSCAN, HSCAN and ZSCAN now reference a dedicated connection that is bound to the Cursor. This allows creation of multiple cursors for different threads without the risk of potentially sharing a single connection. As before the caller is responsible for closing the Cursor correctly after usage. --- .../connection/jedis/JedisConnection.java | 17 ++ .../connection/lettuce/LettuceConnection.java | 19 ++ .../redis/core/DefaultHashOperations.java | 4 +- .../data/redis/core/DefaultSetOperations.java | 4 +- .../redis/core/DefaultZSetOperations.java | 4 +- .../data/redis/core/HashOperations.java | 3 +- .../data/redis/core/RedisOperations.java | 12 ++ .../data/redis/core/RedisTemplate.java | 18 ++ .../data/redis/core/SetOperations.java | 7 +- .../data/redis/core/ZSetOperations.java | 3 + .../jedis/JedisConnectionUnitTestSuite.java | 191 +++++++++++++++++- .../redis/connection/jedis/ScanTests.java | 134 ++++++++++++ .../core/DefaultHashOperationsTests.java | 7 +- .../redis/core/DefaultSetOperationsTests.java | 7 +- .../core/DefaultZSetOperationsTests.java | 7 +- .../redis/core/RedisTemplateUnitTests.java | 31 +++ .../collections/AbstractRedisMapTests.java | 12 +- .../collections/AbstractRedisSetTests.java | 11 +- .../collections/AbstractRedisZSetTest.java | 13 +- .../collections/RedisPropertiesTests.java | 3 +- 20 files changed, 473 insertions(+), 34 deletions(-) create mode 100644 src/test/java/org/springframework/data/redis/connection/jedis/ScanTests.java diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index fac83394d5..3547466dde 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -3790,6 +3790,10 @@ protected ScanIteration doScan(long cursorId, ScanOptions options) { JedisConverters.stringListToByteList().convert(result.getResult())); } + protected void doClose() { + JedisConnection.this.close(); + }; + }.open(); } @@ -3828,6 +3832,10 @@ protected ScanIteration doScan(byte[] key, long cursorId, ScanOptions opt JedisConverters.tuplesToTuples().convert(result.getResult())); } + protected void doClose() { + JedisConnection.this.close(); + }; + }.open(); } @@ -3863,6 +3871,10 @@ protected ScanIteration doScan(byte[] key, long cursorId, ScanOptions op redis.clients.jedis.ScanResult result = jedis.sscan(key, JedisConverters.toBytes(cursorId), params); return new ScanIteration(Long.valueOf(result.getStringCursor()), result.getResult()); } + + protected void doClose() { + JedisConnection.this.close(); + }; }.open(); } @@ -3898,6 +3910,11 @@ protected ScanIteration> doScan(byte[] key, long cursorId, ScanResult> result = jedis.hscan(key, JedisConverters.toBytes(cursorId), params); return new ScanIteration>(Long.valueOf(result.getStringCursor()), result.getResult()); } + + protected void doClose() { + JedisConnection.this.close(); + }; + }.open(); } diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java index 643c30480f..bc7f13ac9e 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java @@ -3719,6 +3719,11 @@ protected ScanIteration doScan(long cursorId, ScanOptions options) { return new ScanIteration(Long.valueOf(nextCursorId), (keys)); } + + protected void doClose() { + LettuceConnection.this.close(); + } + }.open(); } @@ -3759,6 +3764,10 @@ protected ScanIteration> doScan(byte[] key, long cursorId, return new ScanIteration>(Long.valueOf(nextCursorId), values.entrySet()); } + protected void doClose() { + LettuceConnection.this.close(); + } + }.open(); } @@ -3798,6 +3807,11 @@ protected ScanIteration doScan(byte[] key, long cursorId, ScanOptions op List values = failsafeReadScanValues(valueScanCursor.getValues(), null); return new ScanIteration(Long.valueOf(nextCursorId), values); } + + protected void doClose() { + LettuceConnection.this.close(); + } + }.open(); } @@ -3839,6 +3853,11 @@ protected ScanIteration doScan(byte[] key, long cursorId, ScanOptions opt List values = failsafeReadScanValues(result, LettuceConverters.scoredValuesToTupleList()); return new ScanIteration(Long.valueOf(nextCursorId), values); } + + protected void doClose() { + LettuceConnection.this.close(); + } + }.open(); } diff --git a/src/main/java/org/springframework/data/redis/core/DefaultHashOperations.java b/src/main/java/org/springframework/data/redis/core/DefaultHashOperations.java index a8e2ba69b1..25e979802e 100644 --- a/src/main/java/org/springframework/data/redis/core/DefaultHashOperations.java +++ b/src/main/java/org/springframework/data/redis/core/DefaultHashOperations.java @@ -235,7 +235,7 @@ public Map doInRedis(RedisConnection connection) { public Cursor> scan(K key, final ScanOptions options) { final byte[] rawKey = rawKey(key); - return execute(new RedisCallback>>() { + return template.executeWithStickyConnection(new RedisCallback>>() { @Override public Cursor> doInRedis(RedisConnection connection) throws DataAccessException { @@ -268,7 +268,7 @@ public HV setValue(HV value) { }); } - }, true); + }); } } diff --git a/src/main/java/org/springframework/data/redis/core/DefaultSetOperations.java b/src/main/java/org/springframework/data/redis/core/DefaultSetOperations.java index b8afeda487..f70301469c 100644 --- a/src/main/java/org/springframework/data/redis/core/DefaultSetOperations.java +++ b/src/main/java/org/springframework/data/redis/core/DefaultSetOperations.java @@ -255,7 +255,7 @@ public Long doInRedis(RedisConnection connection) { public Cursor scan(K key, final ScanOptions options) { final byte[] rawKey = rawKey(key); - return execute(new RedisCallback>() { + return template.executeWithStickyConnection(new RedisCallback>() { @Override public Cursor doInRedis(RedisConnection connection) throws DataAccessException { @@ -267,7 +267,7 @@ public V convert(byte[] source) { } }); } - }, true); + }); } } diff --git a/src/main/java/org/springframework/data/redis/core/DefaultZSetOperations.java b/src/main/java/org/springframework/data/redis/core/DefaultZSetOperations.java index a098f5d487..bc252a3f5e 100644 --- a/src/main/java/org/springframework/data/redis/core/DefaultZSetOperations.java +++ b/src/main/java/org/springframework/data/redis/core/DefaultZSetOperations.java @@ -409,13 +409,13 @@ public Long doInRedis(RedisConnection connection) { public Cursor> scan(K key, final ScanOptions options) { final byte[] rawKey = rawKey(key); - Cursor cursor = execute(new RedisCallback>() { + Cursor cursor = template.executeWithStickyConnection(new RedisCallback>() { @Override public Cursor doInRedis(RedisConnection connection) throws DataAccessException { return connection.zScan(rawKey, options); } - }, true); + }); return new ConvertingCursor>(cursor, new Converter>() { diff --git a/src/main/java/org/springframework/data/redis/core/HashOperations.java b/src/main/java/org/springframework/data/redis/core/HashOperations.java index 7c3e095988..c5231bebfb 100644 --- a/src/main/java/org/springframework/data/redis/core/HashOperations.java +++ b/src/main/java/org/springframework/data/redis/core/HashOperations.java @@ -150,7 +150,8 @@ public interface HashOperations { RedisOperations getOperations(); /** - * Use a {@link Cursor} to iterate over entries in hash at {@code key}. + * Use a {@link Cursor} to iterate over entries in hash at {@code key}.
+ * Important: Call {@link Cursor#close()} when done to avoid resource leak. * * @since 1.4 * @param key must not be {@literal null}. diff --git a/src/main/java/org/springframework/data/redis/core/RedisOperations.java b/src/main/java/org/springframework/data/redis/core/RedisOperations.java index 74d9d15ee5..7a8de0b86a 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisOperations.java +++ b/src/main/java/org/springframework/data/redis/core/RedisOperations.java @@ -15,6 +15,7 @@ */ package org.springframework.data.redis.core; +import java.io.Closeable; import java.util.Collection; import java.util.Date; import java.util.List; @@ -22,6 +23,7 @@ import java.util.concurrent.TimeUnit; import org.springframework.data.redis.connection.DataType; +import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.core.query.SortQuery; import org.springframework.data.redis.core.script.RedisScript; import org.springframework.data.redis.core.types.RedisClientInfo; @@ -129,6 +131,16 @@ public interface RedisOperations { T execute(RedisScript script, RedisSerializer argsSerializer, RedisSerializer resultSerializer, List keys, Object... args); + /** + * Allocates and binds a new {@link RedisConnection} to the actual return type of the method. It is up to the caller + * to free resources after use. + * + * @param callback must not be {@literal null}. + * @return + * @since 1.8 + */ + T executeWithStickyConnection(RedisCallback callback); + Boolean hasKey(K key); void delete(K key); diff --git a/src/main/java/org/springframework/data/redis/core/RedisTemplate.java b/src/main/java/org/springframework/data/redis/core/RedisTemplate.java index b170cc5f78..dddc361267 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisTemplate.java +++ b/src/main/java/org/springframework/data/redis/core/RedisTemplate.java @@ -15,6 +15,7 @@ */ package org.springframework.data.redis.core; +import java.io.Closeable; import java.lang.reflect.Proxy; import java.util.ArrayList; import java.util.Collection; @@ -304,6 +305,23 @@ public T execute(RedisScript script, RedisSerializer argsSerializer, R return scriptExecutor.execute(script, argsSerializer, resultSerializer, keys, args); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.core.RedisOperations#executeWithStickyConnection(org.springframework.data.redis.core.RedisCallback) + */ + public T executeWithStickyConnection(RedisCallback callback) { + + Assert.isTrue(initialized, "template not initialized; call afterPropertiesSet() before using it"); + Assert.notNull(callback, "Callback object must not be null"); + + RedisConnectionFactory factory = getConnectionFactory(); + + RedisConnection connection = preProcessConnection(RedisConnectionUtils.doGetConnection(factory, true, false, false), + false); + + return callback.doInRedis(connection); + } + private Object executeSession(SessionCallback session) { return session.execute(this); } diff --git a/src/main/java/org/springframework/data/redis/core/SetOperations.java b/src/main/java/org/springframework/data/redis/core/SetOperations.java index b6a3dac56b..377cd12872 100644 --- a/src/main/java/org/springframework/data/redis/core/SetOperations.java +++ b/src/main/java/org/springframework/data/redis/core/SetOperations.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2016 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. @@ -75,8 +75,9 @@ public interface SetOperations { RedisOperations getOperations(); /** - * Iterate over elements in set at {@code key}. - * + * Iterate over elements in set at {@code key}.
+ * Important: Call {@link Cursor#close()} when done to avoid resource leak. + * * @since 1.4 * @param key * @param options diff --git a/src/main/java/org/springframework/data/redis/core/ZSetOperations.java b/src/main/java/org/springframework/data/redis/core/ZSetOperations.java index 880a5ea591..f25fe4379c 100644 --- a/src/main/java/org/springframework/data/redis/core/ZSetOperations.java +++ b/src/main/java/org/springframework/data/redis/core/ZSetOperations.java @@ -136,6 +136,9 @@ public interface TypedTuple extends Comparable> { RedisOperations getOperations(); /** + * Iterate over elements in zset at {@code key}.
+ * Important: Call {@link Cursor#close()} when done to avoid resource leak. + * * @since 1.4 * @param key * @param options diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java index 9114b56605..2ea442890c 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java @@ -1,5 +1,5 @@ /* - * Copyright 2014 the original author or authors. + * Copyright 2014-2016 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. @@ -20,6 +20,10 @@ import static org.mockito.Matchers.*; import static org.mockito.Mockito.*; +import java.io.IOException; +import java.util.Collections; +import java.util.Map.Entry; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -29,11 +33,16 @@ import org.springframework.dao.InvalidDataAccessResourceUsageException; import org.springframework.data.redis.connection.AbstractConnectionUnitTestBase; import org.springframework.data.redis.connection.RedisServerCommands.ShutdownOption; +import org.springframework.data.redis.connection.RedisZSetCommands.Tuple; import org.springframework.data.redis.connection.jedis.JedisConnectionUnitTestSuite.JedisConnectionPipelineUnitTests; import org.springframework.data.redis.connection.jedis.JedisConnectionUnitTestSuite.JedisConnectionUnitTests; +import org.springframework.data.redis.core.Cursor; +import org.springframework.data.redis.core.ScanOptions; import redis.clients.jedis.Client; import redis.clients.jedis.Jedis; +import redis.clients.jedis.ScanParams; +import redis.clients.jedis.ScanResult; /** * @author Christoph Strobl @@ -205,6 +214,122 @@ public void zRangeByScoreShouldThrowExceptionWhenCountExceedsIntegerRange() { connection.zRangeByScore("foo".getBytes(), "foo", "bar", Integer.MAX_VALUE, new Long(Integer.MAX_VALUE) + 1L); } + /** + * @see DATAREDIS-531 + */ + @Test + public void scanShouldKeepTheConnectionOpen() { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).scan(anyString(), + any(ScanParams.class)); + + connection.scan(); + + verify(jedisSpy, never()).close(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void scanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).scan(anyString(), + any(ScanParams.class)); + + Cursor cursor = connection.scan(); + cursor.close(); + + verify(jedisSpy, times(1)).quit(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void sScanShouldKeepTheConnectionOpen() { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).sscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + connection.sScan("foo".getBytes(), ScanOptions.NONE); + + verify(jedisSpy, never()).close(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void sScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).sscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + Cursor cursor = connection.sScan("foo".getBytes(), ScanOptions.NONE); + cursor.close(); + + verify(jedisSpy, times(1)).quit(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void zScanShouldKeepTheConnectionOpen() { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).zscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + connection.zScan("foo".getBytes(), ScanOptions.NONE); + + verify(jedisSpy, never()).close(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void zScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).zscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + Cursor cursor = connection.zScan("foo".getBytes(), ScanOptions.NONE); + cursor.close(); + + verify(jedisSpy, times(1)).quit(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void hScanShouldKeepTheConnectionOpen() { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).hscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + connection.hScan("foo".getBytes(), ScanOptions.NONE); + + verify(jedisSpy, never()).close(); + } + + /** + * @see DATAREDIS-531 + */ + @Test + public void hScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + + doReturn(new ScanResult("0", Collections. emptyList())).when(jedisSpy).hscan(any(byte[].class), + any(byte[].class), any(ScanParams.class)); + + Cursor> cursor = connection.hScan("foo".getBytes(), ScanOptions.NONE); + cursor.close(); + + verify(jedisSpy, times(1)).quit(); + } + } public static class JedisConnectionPipelineUnitTests extends JedisConnectionUnitTests { @@ -267,6 +392,70 @@ public void slaveOfNoOneShouldBeSentCorrectly() { super.slaveOfNoOneShouldBeSentCorrectly(); } + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void scanShouldKeepTheConnectionOpen() { + super.scanShouldKeepTheConnectionOpen(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void scanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + super.scanShouldCloseTheConnectionWhenCursorIsClosed(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void sScanShouldKeepTheConnectionOpen() { + super.sScanShouldKeepTheConnectionOpen(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void sScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + super.sScanShouldCloseTheConnectionWhenCursorIsClosed(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void zScanShouldKeepTheConnectionOpen() { + super.zScanShouldKeepTheConnectionOpen(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void zScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + super.zScanShouldCloseTheConnectionWhenCursorIsClosed(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void hScanShouldKeepTheConnectionOpen() { + super.hScanShouldKeepTheConnectionOpen(); + } + + /** + * @see DATAREDIS-531 + */ + @Test(expected = UnsupportedOperationException.class) + public void hScanShouldCloseTheConnectionWhenCursorIsClosed() throws IOException { + super.hScanShouldCloseTheConnectionWhenCursorIsClosed(); + } + } /** diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/ScanTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/ScanTests.java new file mode 100644 index 0000000000..e2f8cef289 --- /dev/null +++ b/src/test/java/org/springframework/data/redis/connection/jedis/ScanTests.java @@ -0,0 +1,134 @@ +/* + * Copyright 2016 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.redis.connection.jedis; + +import static org.hamcrest.MatcherAssert.*; +import static org.hamcrest.Matchers.*; + +import java.util.Arrays; +import java.util.List; +import java.util.Map.Entry; +import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.springframework.data.redis.ConnectionFactoryTracker; +import org.springframework.data.redis.connection.RedisConnectionFactory; +import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory; +import org.springframework.data.redis.core.BoundHashOperations; +import org.springframework.data.redis.core.Cursor; +import org.springframework.data.redis.core.RedisTemplate; +import org.springframework.data.redis.core.ScanOptions; +import org.springframework.data.redis.core.StringRedisTemplate; + +/** + * @autor Mark Paluch + * @author Christoph Strobl + */ +@RunWith(Parameterized.class) +public class ScanTests { + + RedisConnectionFactory factory; + RedisTemplate redisOperations; + + ThreadPoolExecutor executor = new ThreadPoolExecutor(10, 10, 1, TimeUnit.MINUTES, + new LinkedBlockingDeque()); + + public ScanTests(RedisConnectionFactory factory) { + + this.factory = factory; + ConnectionFactoryTracker.add(factory); + } + + @Parameters + public static List params() { + + JedisConnectionFactory jedisConnectionFactory = new JedisConnectionFactory(); + jedisConnectionFactory.setHostName("127.0.0.1"); + jedisConnectionFactory.setPort(6379); + jedisConnectionFactory.afterPropertiesSet(); + + LettuceConnectionFactory lettuceConnectionFactory = new LettuceConnectionFactory(); + lettuceConnectionFactory.setHostName("127.0.0.1"); + lettuceConnectionFactory.setPort(6379); + lettuceConnectionFactory.afterPropertiesSet(); + + return Arrays. asList(jedisConnectionFactory, lettuceConnectionFactory); + } + + @AfterClass + public static void cleanUp() { + ConnectionFactoryTracker.cleanUp(); + } + + @Before + public void setUp() { + + redisOperations = new StringRedisTemplate(factory); + redisOperations.afterPropertiesSet(); + } + + @Test + public void contextLoads() throws InterruptedException { + + BoundHashOperations hash = redisOperations.boundHashOps("hash"); + final AtomicReference exception = new AtomicReference(); + + // Create some keys so that SCAN requires a while to return all data. + for (int i = 0; i < 10000; i++) { + hash.put("key-" + i, "value"); + } + + // Concurrent access + for (int i = 0; i < 10; i++) { + + executor.submit(new Runnable() { + @Override + public void run() { + try { + + Cursor> cursorMap = redisOperations.boundHashOps("hash") + .scan(ScanOptions.scanOptions().match("*").count(100).build()); + + // This line invokes the lazy SCAN invocation + while (cursorMap.hasNext()) { + cursorMap.next(); + } + cursorMap.close(); + } catch (Exception e) { + exception.set(e); + } + } + }); + } + + // Wait until work is finished + while (executor.getActiveCount() > 0) { + Thread.sleep(100); + } + + executor.shutdown(); + + assertThat(exception.get(), is(nullValue())); + } +} diff --git a/src/test/java/org/springframework/data/redis/core/DefaultHashOperationsTests.java b/src/test/java/org/springframework/data/redis/core/DefaultHashOperationsTests.java index 4f6871da1d..a25fac8d50 100644 --- a/src/test/java/org/springframework/data/redis/core/DefaultHashOperationsTests.java +++ b/src/test/java/org/springframework/data/redis/core/DefaultHashOperationsTests.java @@ -18,9 +18,9 @@ import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import java.io.IOException; import java.util.Arrays; import java.util.Collection; -import java.util.Iterator; import java.util.Map; import org.junit.After; @@ -154,7 +154,7 @@ public void testDelete() { */ @Test @IfProfileValue(name = "redisVersion", value = "2.8+") - public void testHScanReadsValuesFully() { + public void testHScanReadsValuesFully() throws IOException { K key = keyFactory.instance(); HK key1 = hashKeyFactory.instance(); @@ -164,7 +164,7 @@ public void testHScanReadsValuesFully() { hashOps.put(key, key1, val1); hashOps.put(key, key2, val2); - Iterator> it = hashOps.scan(key, ScanOptions.scanOptions().count(1).build()); + Cursor> it = hashOps.scan(key, ScanOptions.scanOptions().count(1).build()); long count = 0; while (it.hasNext()) { @@ -174,6 +174,7 @@ public void testHScanReadsValuesFully() { count++; } + it.close(); assertThat(count, is(hashOps.size(key))); } diff --git a/src/test/java/org/springframework/data/redis/core/DefaultSetOperationsTests.java b/src/test/java/org/springframework/data/redis/core/DefaultSetOperationsTests.java index f5453758d1..034fd5a983 100644 --- a/src/test/java/org/springframework/data/redis/core/DefaultSetOperationsTests.java +++ b/src/test/java/org/springframework/data/redis/core/DefaultSetOperationsTests.java @@ -20,10 +20,10 @@ import static org.junit.Assume.*; import static org.springframework.data.redis.matcher.RedisTestMatchers.*; +import java.io.IOException; import java.util.Collection; import java.util.Collections; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Set; @@ -216,7 +216,7 @@ public void testRemove() { @Test @SuppressWarnings("unchecked") @IfProfileValue(name = "redisVersion", value = "2.8+") - public void testSSCanReadsValuesFully() { + public void testSSCanReadsValuesFully() throws IOException { K key = keyFactory.instance(); V v1 = valueFactory.instance(); @@ -224,12 +224,13 @@ public void testSSCanReadsValuesFully() { V v3 = valueFactory.instance(); setOps.add(key, v1, v2, v3); - Iterator it = setOps.scan(key, ScanOptions.scanOptions().count(1).build()); + Cursor it = setOps.scan(key, ScanOptions.scanOptions().count(1).build()); long count = 0; while (it.hasNext()) { assertThat(it.next(), anyOf(equalTo(v1), equalTo(v2), equalTo(v3))); count++; } + it.close(); assertThat(count, is(setOps.size(key))); } diff --git a/src/test/java/org/springframework/data/redis/core/DefaultZSetOperationsTests.java b/src/test/java/org/springframework/data/redis/core/DefaultZSetOperationsTests.java index b03c38d68d..31d6186a3e 100644 --- a/src/test/java/org/springframework/data/redis/core/DefaultZSetOperationsTests.java +++ b/src/test/java/org/springframework/data/redis/core/DefaultZSetOperationsTests.java @@ -22,10 +22,10 @@ import static org.junit.Assume.*; import static org.springframework.data.redis.matcher.RedisTestMatchers.*; +import java.io.IOException; import java.util.Collection; import java.util.Collections; import java.util.HashSet; -import java.util.Iterator; import java.util.LinkedHashSet; import java.util.Set; @@ -355,7 +355,7 @@ public void sizeRetrievesDataCorrectly() { */ @Test @IfProfileValue(name = "redisVersion", value = "2.8+") - public void testZScanShouldReadEntireValueRange() { + public void testZScanShouldReadEntireValueRange() throws IOException { K key = keyFactory.instance(); @@ -374,12 +374,13 @@ public void testZScanShouldReadEntireValueRange() { zSetOps.add(key, values); int count = 0; - Iterator> it = zSetOps.scan(key, ScanOptions.scanOptions().count(2).build()); + Cursor> it = zSetOps.scan(key, ScanOptions.scanOptions().count(2).build()); while (it.hasNext()) { assertThat(it.next(), anyOf(equalTo(tuple1), equalTo(tuple2), equalTo(tuple3))); count++; } + it.close(); assertThat(count, equalTo(3)); } } diff --git a/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java b/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java index 9806bcaa11..0a8a9a1dfe 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java +++ b/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java @@ -23,11 +23,13 @@ import java.io.Serializable; +import org.hamcrest.core.IsSame; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.dao.DataAccessException; import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.data.redis.serializer.JdkSerializationRedisSerializer; @@ -94,8 +96,37 @@ public void templateShouldPassOnAndUseResoureLoaderClassLoaderToDefaultJdkSerial assertThat(deserialized.getClass().getClassLoader(), is((ClassLoader) scl)); } + /** + * @see DATAREDIS-531 + */ + @Test + public void executeWithStickyConnectionShouldNotCloseConnectionWhenDone() { + + CapturingCallback callback = new CapturingCallback(); + template.executeWithStickyConnection(callback); + + assertThat(callback.getConnection(), IsSame.sameInstance(redisConnectionMock)); + verify(redisConnectionMock, never()).close(); + } + static class SomeArbitrarySeriaizableObject implements Serializable { private static final long serialVersionUID = -5973659324040506423L; } + static class CapturingCallback implements RedisCallback> { + + private RedisConnection connection; + + @Override + public Cursor doInRedis(RedisConnection connection) throws DataAccessException { + this.connection = connection; + return null; + } + + public RedisConnection getConnection() { + return connection; + } + + } + } diff --git a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisMapTests.java b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisMapTests.java index cf540b0cb2..3e7da39bae 100644 --- a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisMapTests.java +++ b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisMapTests.java @@ -20,11 +20,11 @@ import static org.junit.Assume.*; import static org.springframework.data.redis.matcher.RedisTestMatchers.*; +import java.io.IOException; import java.text.DecimalFormat; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; @@ -47,6 +47,7 @@ import org.springframework.data.redis.connection.ConnectionUtils; import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.connection.RedisConnectionFactory; +import org.springframework.data.redis.core.Cursor; import org.springframework.data.redis.core.RedisCallback; import org.springframework.data.redis.core.RedisOperations; import org.springframework.data.redis.core.RedisTemplate; @@ -493,7 +494,7 @@ public void testReplaceNullValue() { @Test @IfProfileValue(name = "redisVersion", value = "2.8+") @WithRedisDriver({ RedisDriver.JEDIS, RedisDriver.LETTUCE }) - public void testScanWorksCorrectly() { + public void testScanWorksCorrectly() throws IOException { K k1 = getKey(); K k2 = getKey(); @@ -504,11 +505,12 @@ public void testScanWorksCorrectly() { map.put(k1, v1); map.put(k2, v2); - Iterator> it = map.scan(); - while (it.hasNext()) { - Entry entry = it.next(); + Cursor> cursor = (Cursor>) map.scan(); + while (cursor.hasNext()) { + Entry entry = cursor.next(); assertThat(entry.getKey(), anyOf(equalTo(k1), equalTo(k2))); assertThat(entry.getValue(), anyOf(equalTo(v1), equalTo(v2))); } + cursor.close(); } } diff --git a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisSetTests.java b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisSetTests.java index 078fb17482..b373cf8d12 100644 --- a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisSetTests.java +++ b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisSetTests.java @@ -19,6 +19,7 @@ import static org.junit.Assert.*; import static org.springframework.data.redis.matcher.RedisTestMatchers.*; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; @@ -31,6 +32,7 @@ import org.springframework.data.redis.ObjectFactory; import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.data.redis.core.BoundSetOperations; +import org.springframework.data.redis.core.Cursor; import org.springframework.data.redis.core.RedisTemplate; import org.springframework.data.redis.test.util.MinimumRedisVersionRule; import org.springframework.data.redis.test.util.RedisClientRule; @@ -308,14 +310,15 @@ public void testToArrayWithGenerics() { @IfProfileValue(name = "redisVersion", value = "2.8+") @Test @WithRedisDriver({ RedisDriver.JEDIS, RedisDriver.LETTUCE }) - public void testScanWorksCorrectly() { + public void testScanWorksCorrectly() throws IOException { Object[] expectedArray = new Object[] { getT(), getT(), getT() }; collection.addAll((List) Arrays.asList(expectedArray)); - Iterator it = set.scan(); - while (it.hasNext()) { - assertThat(it.next(), anyOf(equalTo(expectedArray[0]), equalTo(expectedArray[1]), equalTo(expectedArray[2]))); + Cursor cursor = (Cursor) set.scan(); + while (cursor.hasNext()) { + assertThat(cursor.next(), anyOf(equalTo(expectedArray[0]), equalTo(expectedArray[1]), equalTo(expectedArray[2]))); } + cursor.close(); } } diff --git a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisZSetTest.java b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisZSetTest.java index f95fc867d4..b762ee0f9e 100644 --- a/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisZSetTest.java +++ b/src/test/java/org/springframework/data/redis/support/collections/AbstractRedisZSetTest.java @@ -20,6 +20,7 @@ import static org.junit.Assume.*; import static org.springframework.data.redis.matcher.RedisTestMatchers.*; +import java.io.IOException; import java.util.Arrays; import java.util.Iterator; import java.util.NoSuchElementException; @@ -37,6 +38,7 @@ import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.data.redis.connection.RedisZSetCommands; import org.springframework.data.redis.core.BoundZSetOperations; +import org.springframework.data.redis.core.Cursor; import org.springframework.data.redis.core.RedisTemplate; import org.springframework.data.redis.core.ZSetOperations.TypedTuple; import org.springframework.data.redis.test.util.MinimumRedisVersionRule; @@ -645,7 +647,7 @@ public void testToArrayWithGenerics() { @IfProfileValue(name = "redisVersion", value = "2.8+") @Test @WithRedisDriver({ RedisDriver.JEDIS, RedisDriver.LETTUCE }) - public void testScanWorksCorrectly() { + public void testScanWorksCorrectly() throws IOException { T t1 = getT(); T t2 = getT(); @@ -657,9 +659,12 @@ public void testScanWorksCorrectly() { zSet.add(t3, 3); zSet.add(t4, 4); - Iterator it = zSet.scan(); - while (it.hasNext()) { - assertThat(it.next(), anyOf(equalTo(t1), equalTo(t2), equalTo(t3), equalTo(t4))); + Cursor cursor = (Cursor) zSet.scan(); + while (cursor.hasNext()) { + assertThat(cursor.next(), anyOf(equalTo(t1), equalTo(t2), equalTo(t3), equalTo(t4))); } + + cursor.close(); + } } diff --git a/src/test/java/org/springframework/data/redis/support/collections/RedisPropertiesTests.java b/src/test/java/org/springframework/data/redis/support/collections/RedisPropertiesTests.java index 3561107413..3b0588caf7 100644 --- a/src/test/java/org/springframework/data/redis/support/collections/RedisPropertiesTests.java +++ b/src/test/java/org/springframework/data/redis/support/collections/RedisPropertiesTests.java @@ -18,6 +18,7 @@ import static org.junit.Assert.*; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.io.InputStream; import java.io.PrintWriter; import java.io.StringWriter; @@ -225,7 +226,7 @@ public void testStringPropertyNames() throws Exception { @Override @Test(expected = UnsupportedOperationException.class) - public void testScanWorksCorrectly() { + public void testScanWorksCorrectly() throws IOException { super.testScanWorksCorrectly(); } From 6e0d075e27b24341712f28ccffc34c402a277f60 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 5 Sep 2016 08:52:17 +0200 Subject: [PATCH 3/3] DATAREDIS-531 - Polishing. Use quit method for close verification in tests. Refine static imports. Fix spelling. --- .../connection/jedis/JedisConnectionUnitTestSuite.java | 9 ++++----- .../data/redis/core/RedisTemplateUnitTests.java | 10 ++++------ 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java index 2ea442890c..150954be79 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionUnitTestSuite.java @@ -225,7 +225,7 @@ public void scanShouldKeepTheConnectionOpen() { connection.scan(); - verify(jedisSpy, never()).close(); + verify(jedisSpy, never()).quit(); } /** @@ -254,7 +254,7 @@ public void sScanShouldKeepTheConnectionOpen() { connection.sScan("foo".getBytes(), ScanOptions.NONE); - verify(jedisSpy, never()).close(); + verify(jedisSpy, never()).quit(); } /** @@ -283,7 +283,7 @@ public void zScanShouldKeepTheConnectionOpen() { connection.zScan("foo".getBytes(), ScanOptions.NONE); - verify(jedisSpy, never()).close(); + verify(jedisSpy, never()).quit(); } /** @@ -312,7 +312,7 @@ public void hScanShouldKeepTheConnectionOpen() { connection.hScan("foo".getBytes(), ScanOptions.NONE); - verify(jedisSpy, never()).close(); + verify(jedisSpy, never()).quit(); } /** @@ -467,6 +467,5 @@ public MockedClientJedis(String host, Client client) { super(host); this.client = client; } - } } diff --git a/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java b/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java index 0a8a9a1dfe..f280356769 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java +++ b/src/test/java/org/springframework/data/redis/core/RedisTemplateUnitTests.java @@ -17,13 +17,13 @@ import static org.hamcrest.core.Is.*; import static org.hamcrest.core.IsNull.*; +import static org.hamcrest.core.IsSame.*; import static org.junit.Assert.*; import static org.mockito.Matchers.*; import static org.mockito.Mockito.*; import java.io.Serializable; -import org.hamcrest.core.IsSame; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -89,7 +89,7 @@ public void templateShouldPassOnAndUseResoureLoaderClassLoaderToDefaultJdkSerial template.afterPropertiesSet(); when(redisConnectionMock.get(any(byte[].class))) - .thenReturn(new JdkSerializationRedisSerializer().serialize(new SomeArbitrarySeriaizableObject())); + .thenReturn(new JdkSerializationRedisSerializer().serialize(new SomeArbitrarySerializableObject())); Object deserialized = template.opsForValue().get("spring"); assertThat(deserialized, notNullValue()); @@ -105,11 +105,11 @@ public void executeWithStickyConnectionShouldNotCloseConnectionWhenDone() { CapturingCallback callback = new CapturingCallback(); template.executeWithStickyConnection(callback); - assertThat(callback.getConnection(), IsSame.sameInstance(redisConnectionMock)); + assertThat(callback.getConnection(), sameInstance(redisConnectionMock)); verify(redisConnectionMock, never()).close(); } - static class SomeArbitrarySeriaizableObject implements Serializable { + static class SomeArbitrarySerializableObject implements Serializable { private static final long serialVersionUID = -5973659324040506423L; } @@ -126,7 +126,5 @@ public Cursor doInRedis(RedisConnection connection) throws DataAccessExc public RedisConnection getConnection() { return connection; } - } - }