From 803fc814e88ec99e4726e0fa100534271927742a Mon Sep 17 00:00:00 2001 From: Nikita Date: Fri, 26 May 2017 12:01:08 +0300 Subject: [PATCH] Iterator infinite scan bug fixed #885 --- .../org/redisson/RedissonBaseIterator.java | 30 +++---------- .../org/redisson/RedissonBaseMapIterator.java | 32 ++++++++------ .../java/org/redisson/RedissonKeysTest.java | 1 + .../test/java/org/redisson/RedissonTest.java | 43 ++++++++++++++++++- 4 files changed, 66 insertions(+), 40 deletions(-) diff --git a/redisson/src/main/java/org/redisson/RedissonBaseIterator.java b/redisson/src/main/java/org/redisson/RedissonBaseIterator.java index 9faa1d2b2..4410270cf 100644 --- a/redisson/src/main/java/org/redisson/RedissonBaseIterator.java +++ b/redisson/src/main/java/org/redisson/RedissonBaseIterator.java @@ -42,7 +42,6 @@ abstract class RedissonBaseIterator implements Iterator { private boolean finished; private boolean currentElementRemoved; - private boolean removeExecuted; private V value; @Override @@ -53,7 +52,6 @@ abstract class RedissonBaseIterator implements Iterator { free(lastValues); currentElementRemoved = false; - removeExecuted = false; client = null; firstValues = null; lastValues = null; @@ -64,9 +62,7 @@ abstract class RedissonBaseIterator implements Iterator { } finished = false; } - long prevIterPos; do { - prevIterPos = nextIterPos; ListScanResult res = iterator(client, nextIterPos); if (lastValues != null) { free(lastValues); @@ -82,7 +78,6 @@ abstract class RedissonBaseIterator implements Iterator { client = null; firstValues = null; nextIterPos = 0; - prevIterPos = -1; } } else { if (firstValues.isEmpty()) { @@ -93,39 +88,30 @@ abstract class RedissonBaseIterator implements Iterator { client = null; firstValues = null; nextIterPos = 0; - prevIterPos = -1; continue; } if (res.getPos() == 0) { + free(firstValues); + free(lastValues); + finished = true; return false; } } - } else if (lastValues.removeAll(firstValues)) { + } else if (lastValues.removeAll(firstValues) + || (lastValues.isEmpty() && nextIterPos == 0)) { free(firstValues); free(lastValues); currentElementRemoved = false; - removeExecuted = false; client = null; firstValues = null; lastValues = null; nextIterPos = 0; - prevIterPos = -1; if (tryAgain()) { continue; } - finished = true; - return false; - } else if (lastValues.isEmpty() && res.getPos() == 0) { - if (tryAgain()) { - client = null; - firstValues = null; - nextIterPos = 0; - prevIterPos = -1; - continue; - } finished = true; return false; @@ -133,10 +119,7 @@ abstract class RedissonBaseIterator implements Iterator { } lastIter = res.getValues().iterator(); nextIterPos = res.getPos(); - } while (!lastIter.hasNext() && nextIterPos != prevIterPos); - if (prevIterPos == nextIterPos && !removeExecuted) { - finished = true; - } + } while (!lastIter.hasNext()); } return lastIter.hasNext(); } @@ -188,7 +171,6 @@ abstract class RedissonBaseIterator implements Iterator { lastIter.remove(); remove(value); currentElementRemoved = true; - removeExecuted = true; } abstract void remove(V value); diff --git a/redisson/src/main/java/org/redisson/RedissonBaseMapIterator.java b/redisson/src/main/java/org/redisson/RedissonBaseMapIterator.java index f2924e205..de2363299 100644 --- a/redisson/src/main/java/org/redisson/RedissonBaseMapIterator.java +++ b/redisson/src/main/java/org/redisson/RedissonBaseMapIterator.java @@ -28,6 +28,14 @@ import org.redisson.client.protocol.decoder.ScanObjectEntry; import io.netty.buffer.ByteBuf; +/** + * + * @author Nikita Koksharov + * + * @param key type + * @param value type + * @param loaded value type + */ public abstract class RedissonBaseMapIterator implements Iterator { private Map firstValues; @@ -38,7 +46,6 @@ public abstract class RedissonBaseMapIterator implements Iterator { private boolean finished; private boolean currentElementRemoved; - private boolean removeExecuted; protected Map.Entry entry; @Override @@ -49,7 +56,6 @@ public abstract class RedissonBaseMapIterator implements Iterator { free(lastValues); currentElementRemoved = false; - removeExecuted = false; client = null; firstValues = null; lastValues = null; @@ -60,15 +66,15 @@ public abstract class RedissonBaseMapIterator implements Iterator { } finished = false; } - long prevIterPos; do { - prevIterPos = nextIterPos; MapScanResult res = iterator(); if (lastValues != null) { free(lastValues); } + lastValues = convert(res.getMap()); client = res.getRedisClient(); + if (nextIterPos == 0 && firstValues == null) { firstValues = lastValues; lastValues = null; @@ -76,7 +82,6 @@ public abstract class RedissonBaseMapIterator implements Iterator { client = null; firstValues = null; nextIterPos = 0; - prevIterPos = -1; } } else { if (firstValues.isEmpty()) { @@ -87,38 +92,38 @@ public abstract class RedissonBaseMapIterator implements Iterator { client = null; firstValues = null; nextIterPos = 0; - prevIterPos = -1; continue; } if (res.getPos() == 0) { + free(firstValues); + free(lastValues); + finished = true; return false; } } - } else if (lastValues.keySet().removeAll(firstValues.keySet())) { + } else if (lastValues.keySet().removeAll(firstValues.keySet()) + || (lastValues.isEmpty() && nextIterPos == 0)) { free(firstValues); free(lastValues); currentElementRemoved = false; - removeExecuted = false; + client = null; firstValues = null; lastValues = null; nextIterPos = 0; - prevIterPos = -1; if (tryAgain()) { continue; } + finished = true; return false; } } lastIter = res.getMap().entrySet().iterator(); nextIterPos = res.getPos(); - } while (!lastIter.hasNext() && nextIterPos != prevIterPos); - if (prevIterPos == nextIterPos && !removeExecuted) { - finished = true; - } + } while (!lastIter.hasNext()); } return lastIter.hasNext(); @@ -184,7 +189,6 @@ public abstract class RedissonBaseMapIterator implements Iterator { lastIter.remove(); removeKey(); currentElementRemoved = true; - removeExecuted = true; entry = null; } diff --git a/redisson/src/test/java/org/redisson/RedissonKeysTest.java b/redisson/src/test/java/org/redisson/RedissonKeysTest.java index c19d167c5..6c13be4d2 100644 --- a/redisson/src/test/java/org/redisson/RedissonKeysTest.java +++ b/redisson/src/test/java/org/redisson/RedissonKeysTest.java @@ -66,6 +66,7 @@ public class RedissonKeysTest extends BaseTest { for (int i = 0; i < 115; i++) { String key = "key" + Math.random(); RBucket bucket = redisson.getBucket(key); + keys.add(key); bucket.set("someValue"); } diff --git a/redisson/src/test/java/org/redisson/RedissonTest.java b/redisson/src/test/java/org/redisson/RedissonTest.java index c6be7a759..38843b04a 100644 --- a/redisson/src/test/java/org/redisson/RedissonTest.java +++ b/redisson/src/test/java/org/redisson/RedissonTest.java @@ -6,6 +6,7 @@ import static org.redisson.BaseTest.createInstance; import java.io.IOException; import java.net.InetSocketAddress; +import java.util.Arrays; import java.util.Collections; import java.util.Iterator; import java.util.Map; @@ -33,10 +34,13 @@ import org.redisson.client.RedisConnectionException; import org.redisson.client.RedisException; import org.redisson.client.RedisOutOfMemoryException; import org.redisson.client.protocol.decoder.ListScanResult; +import org.redisson.client.protocol.decoder.ScanObjectEntry; import org.redisson.codec.SerializationCodec; import org.redisson.config.Config; import org.redisson.connection.ConnectionListener; +import io.netty.buffer.Unpooled; + public class RedissonTest { protected RedissonClient redisson; @@ -76,7 +80,7 @@ public class RedissonTest { } @Test - public void testIterator() { + public void testIteratorNotLooped() { RedissonBaseIterator iter = new RedissonBaseIterator() { int i; @Override @@ -101,6 +105,41 @@ public class RedissonTest { Assert.assertFalse(iter.hasNext()); } + @Test + public void testIteratorNotLooped2() { + RedissonBaseIterator iter = new RedissonBaseIterator() { + int i; + @Override + ListScanResult iterator(InetSocketAddress client, long nextIterPos) { + i++; + if (i == 1) { + return new ListScanResult(14L, Arrays.asList(new ScanObjectEntry(Unpooled.wrappedBuffer(new byte[] {1}), 1))); + } + if (i == 2) { + return new ListScanResult(7L, Collections.emptyList()); + } + if (i == 3) { + return new ListScanResult(0L, Collections.emptyList()); + } + if (i == 4) { + return new ListScanResult(14L, Collections.emptyList()); + } + Assert.fail(); + return null; + } + + @Override + void remove(Integer value) { + } + + }; + + Assert.assertTrue(iter.hasNext()); + assertThat(iter.next()).isEqualTo(1); + Assert.assertFalse(iter.hasNext()); + } + + @BeforeClass public static void beforeClass() throws IOException, InterruptedException { if (!RedissonRuntimeEnvironment.isTravis) { @@ -250,7 +289,7 @@ public class RedissonTest { Assert.assertEquals(0, pp.stop()); - await().atMost(1, TimeUnit.SECONDS).until(() -> assertThat(connectCounter.get()).isEqualTo(1)); + await().atMost(2, TimeUnit.SECONDS).until(() -> assertThat(connectCounter.get()).isEqualTo(1)); await().until(() -> assertThat(disconnectCounter.get()).isEqualTo(1)); }