From 6ea1959cc5308389f5db6d430c3fc8aa122bd32f Mon Sep 17 00:00:00 2001 From: nilskp Date: Fri, 3 Oct 2014 15:36:09 -0500 Subject: [PATCH 1/3] Fixes xetorthio/jedis#758 --- .../java/redis/clients/jedis/Pipeline.java | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/main/java/redis/clients/jedis/Pipeline.java b/src/main/java/redis/clients/jedis/Pipeline.java index c79c969..c3caefc 100644 --- a/src/main/java/redis/clients/jedis/Pipeline.java +++ b/src/main/java/redis/clients/jedis/Pipeline.java @@ -88,17 +88,18 @@ public class Pipeline extends MultiKeyPipelineBase { } /** - * Syncronize pipeline by reading all responses. This operation close the + * Synchronize pipeline by reading all responses. This operation close the * pipeline. In order to get return values from pipelined commands, capture * the different Response of the commands you execute. */ public void sync() { - List unformatted = client.getMany(getPipelinedResponseLength()); - for (Object o : unformatted) { - generateResponse(o); - } + if (client.isConnected()) { + List unformatted = client.getMany(getPipelinedResponseLength()); + for (Object o : unformatted) { + generateResponse(o); + } + } } - /** * Syncronize pipeline by reading all responses. This operation close the * pipeline. Whenever possible try to avoid using this version and use @@ -108,17 +109,21 @@ public class Pipeline extends MultiKeyPipelineBase { * @return A list of all the responses in the order you executed them. */ public List syncAndReturnAll() { - List unformatted = client.getMany(getPipelinedResponseLength()); - List formatted = new ArrayList(); + if (client.isConnected()) { + List unformatted = client.getMany(getPipelinedResponseLength()); + List formatted = new ArrayList(); - for (Object o : unformatted) { - try { - formatted.add(generateResponse(o).get()); - } catch (JedisDataException e) { - formatted.add(e); - } - } - return formatted; + for (Object o : unformatted) { + try { + formatted.add(generateResponse(o).get()); + } catch (JedisDataException e) { + formatted.add(e); + } + } + return formatted; + } else { + return java.util.Collections.emptyList(); + } } public Response discard() { From 745f1b1e4e180460d83423e83e46583103f1ad34 Mon Sep 17 00:00:00 2001 From: nilskp Date: Sat, 4 Oct 2014 11:22:45 -0500 Subject: [PATCH 2/3] Updated fix to xetorthio/jedis#758 --- src/main/java/redis/clients/jedis/Pipeline.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/java/redis/clients/jedis/Pipeline.java b/src/main/java/redis/clients/jedis/Pipeline.java index c3caefc..5dc3179 100644 --- a/src/main/java/redis/clients/jedis/Pipeline.java +++ b/src/main/java/redis/clients/jedis/Pipeline.java @@ -93,15 +93,15 @@ public class Pipeline extends MultiKeyPipelineBase { * the different Response of the commands you execute. */ public void sync() { - if (client.isConnected()) { + if (getPipelinedResponseLength() > 0) { List unformatted = client.getMany(getPipelinedResponseLength()); for (Object o : unformatted) { generateResponse(o); } - } + } } /** - * Syncronize pipeline by reading all responses. This operation close the + * Synchronize pipeline by reading all responses. This operation close the * pipeline. Whenever possible try to avoid using this version and use * Pipeline.sync() as it won't go through all the responses and generate the * right response type (usually it is a waste of time). @@ -109,10 +109,9 @@ public class Pipeline extends MultiKeyPipelineBase { * @return A list of all the responses in the order you executed them. */ public List syncAndReturnAll() { - if (client.isConnected()) { + if (getPipelinedResponseLength() > 0) { List unformatted = client.getMany(getPipelinedResponseLength()); List formatted = new ArrayList(); - for (Object o : unformatted) { try { formatted.add(generateResponse(o).get()); From a8891cb00cc1c6649d68dc70005835a2999a461f Mon Sep 17 00:00:00 2001 From: Jungtaek Lim Date: Sun, 5 Oct 2014 14:36:14 +0900 Subject: [PATCH 3/3] Add unit tests to check pipeline sync without any commands --- .../clients/jedis/tests/PipeliningTest.java | 19 ++++++++++++ .../jedis/tests/ShardedJedisPipelineTest.java | 30 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/test/java/redis/clients/jedis/tests/PipeliningTest.java b/src/test/java/redis/clients/jedis/tests/PipeliningTest.java index eb336bd..8b5732c 100644 --- a/src/test/java/redis/clients/jedis/tests/PipeliningTest.java +++ b/src/test/java/redis/clients/jedis/tests/PipeliningTest.java @@ -438,6 +438,25 @@ public class PipeliningTest extends Assert { assertTrue(result.get(3) instanceof JedisDataException); } + + @Test + public void testSyncWithNoCommandQueued() { + // we need to test with fresh instance of Jedis + Jedis jedis2 = new Jedis(hnp.getHost(), hnp.getPort(), 500); + + Pipeline pipeline = jedis2.pipelined(); + pipeline.sync(); + + jedis2.close(); + + jedis2 = new Jedis(hnp.getHost(), hnp.getPort(), 500); + + pipeline = jedis2.pipelined(); + List resp = pipeline.syncAndReturnAll(); + assertTrue(resp.isEmpty()); + + jedis2.close(); + } private void verifyHasBothValues(String firstKey, String secondKey, String value1, String value2) { assertFalse(firstKey.equals(secondKey)); diff --git a/src/test/java/redis/clients/jedis/tests/ShardedJedisPipelineTest.java b/src/test/java/redis/clients/jedis/tests/ShardedJedisPipelineTest.java index bf9b2bd..a6dfaab 100644 --- a/src/test/java/redis/clients/jedis/tests/ShardedJedisPipelineTest.java +++ b/src/test/java/redis/clients/jedis/tests/ShardedJedisPipelineTest.java @@ -4,6 +4,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import java.io.UnsupportedEncodingException; import java.util.ArrayList; @@ -18,6 +19,7 @@ import org.junit.Test; import redis.clients.jedis.HostAndPort; import redis.clients.jedis.Jedis; import redis.clients.jedis.JedisShardInfo; +import redis.clients.jedis.Pipeline; import redis.clients.jedis.Response; import redis.clients.jedis.ShardedJedis; import redis.clients.jedis.ShardedJedisPipeline; @@ -130,4 +132,32 @@ public class ShardedJedisPipelineTest { p.sync(); assertNull(shouldNotExist.get()); } + + @Test + public void testSyncWithNoCommandQueued() { + JedisShardInfo shardInfo1 = new JedisShardInfo(redis1.getHost(), + redis1.getPort()); + JedisShardInfo shardInfo2 = new JedisShardInfo(redis2.getHost(), + redis2.getPort()); + shardInfo1.setPassword("foobared"); + shardInfo2.setPassword("foobared"); + List shards = new ArrayList(); + shards.add(shardInfo1); + shards.add(shardInfo2); + + ShardedJedis jedis2 = new ShardedJedis(shards); + + ShardedJedisPipeline pipeline = jedis2.pipelined(); + pipeline.sync(); + + jedis2.close(); + + jedis2 = new ShardedJedis(shards); + pipeline = jedis2.pipelined(); + List resp = pipeline.syncAndReturnAll(); + assertTrue(resp.isEmpty()); + + jedis2.close(); + } + }