From db7b07788fb1c8df0873cc2d0313ff5694527a47 Mon Sep 17 00:00:00 2001 From: Jennifer Hickey Date: Sun, 26 May 2013 15:02:54 -0700 Subject: [PATCH] Fix Lettuce compareAndSet DATAREDIS-122 - exec() now returns null if value modified by other thread - Use same connection for watch/unwatch and txs --- .../connection/lettuce/LettuceConnection.java | 52 ++++++++++++------- .../AbstractConnectionIntegrationTests.java | 1 - .../LettuceConnectionIntegrationTests.java | 14 ++--- ...uceConnectionPipelineIntegrationTests.java | 43 ++++++++++++--- ...ConnectionTransactionIntegrationTests.java | 10 ++-- ...RjcConnectionPipelineIntegrationTests.java | 4 ++ .../redis/support/BoundKeyOperationsTest.java | 3 +- .../support/atomic/RedisAtomicTests.java | 5 +- 8 files changed, 84 insertions(+), 48 deletions(-) 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 9a854c4cc..b9dad8b19 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 @@ -489,10 +489,10 @@ public class LettuceConnection implements RedisConnection { isMulti = false; try { if (isPipelined()) { - pipeline(asyncTxConn.discard()); + pipeline(getAsyncTxConnection().discard()); return; } - txConn.discard(); + getTxConnection().discard(); } catch (Exception ex) { throw convertLettuceAccessException(ex); } @@ -503,10 +503,14 @@ public class LettuceConnection implements RedisConnection { isMulti = false; try { if (isPipelined()) { - asyncTxConn.exec(); + getAsyncTxConnection().exec(); return null; } - return txConn.exec(); + List results = getTxConnection().exec(); + if(results.isEmpty()) { + return null; + } + return results; } catch (Exception ex) { throw convertLettuceAccessException(ex); } @@ -572,17 +576,11 @@ public class LettuceConnection implements RedisConnection { } isMulti = true; try { - if(asyncTxConn == null) { - asyncTxConn = client.connectAsync(LettuceUtils.CODEC); - } - if(txConn == null) { - txConn = new com.lambdaworks.redis.RedisConnection(asyncTxConn); - } if (isPipelined()) { - pipeline(asyncTxConn.multi()); + pipeline(getAsyncTxConnection().multi()); return; } - txConn.multi(); + getTxConnection().multi(); } catch (Exception ex) { throw convertLettuceAccessException(ex); } @@ -688,10 +686,10 @@ public class LettuceConnection implements RedisConnection { public void unwatch() { try { if (isPipelined()) { - pipeline(getAsyncConnection().unwatch()); + pipeline(getAsyncTxConnection().unwatch()); return; } - getConnection().unwatch(); + getTxConnection().unwatch(); } catch (Exception ex) { throw convertLettuceAccessException(ex); } @@ -701,11 +699,11 @@ public class LettuceConnection implements RedisConnection { public void watch(byte[]... keys) { try { if (isPipelined()) { - pipeline(getAsyncConnection().watch(keys)); + pipeline(getAsyncTxConnection().watch(keys)); return; } else { - getConnection().watch(keys); + getTxConnection().watch(keys); } } catch (Exception ex) { throw convertLettuceAccessException(ex); @@ -1922,21 +1920,21 @@ public class LettuceConnection implements RedisConnection { private RedisAsyncConnection getAsyncConnection() { if(isQueueing()) { - return asyncTxConn; + return getAsyncTxConnection(); } return asyncConn; } private com.lambdaworks.redis.RedisConnection getConnection() { if(isQueueing()) { - return txConn; + return getTxConnection(); } return con; } private RedisAsyncConnection getAsyncBlockingConnection() { if(isQueueing()) { - return asyncTxConn; + return getAsyncTxConnection(); } if(asyncBlockingConn == null) { asyncBlockingConn = client.connectAsync(LettuceUtils.CODEC); @@ -1946,11 +1944,25 @@ public class LettuceConnection implements RedisConnection { private com.lambdaworks.redis.RedisConnection getBlockingConnection() { if(isQueueing()) { - return txConn; + return getTxConnection(); } if(blockingConn == null) { blockingConn = new com.lambdaworks.redis.RedisConnection(getAsyncBlockingConnection()); } return blockingConn; } + + private RedisAsyncConnection getAsyncTxConnection() { + if(asyncTxConn == null) { + asyncTxConn = client.connectAsync(LettuceUtils.CODEC); + } + return asyncTxConn; + } + + private com.lambdaworks.redis.RedisConnection getTxConnection() { + if(txConn == null) { + txConn = new com.lambdaworks.redis.RedisConnection(getAsyncTxConnection()); + } + return txConn; + } } \ No newline at end of file diff --git a/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java index 78038ffbd..dcf2fc5ce 100644 --- a/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java @@ -400,7 +400,6 @@ public abstract class AbstractConnectionIntegrationTests { public void testWatch() throws Exception { connection.set("testitnow", "willdo"); connection.watch("testitnow".getBytes()); - connection.get("testitnow"); DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( connectionFactory.getConnection()); conn2.set("testitnow", "something"); diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java index 19818b89e..601f8210c 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionIntegrationTests.java @@ -16,6 +16,9 @@ package org.springframework.data.redis.connection.lettuce; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + import java.util.List; import org.junit.Ignore; @@ -26,8 +29,6 @@ import org.springframework.data.redis.connection.AbstractConnectionIntegrationTe import org.springframework.data.redis.connection.DefaultStringRedisConnection; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.fail; /** * Integration test of {@link LettuceConnection} @@ -40,14 +41,6 @@ import static org.junit.Assert.fail; @ContextConfiguration public class LettuceConnectionIntegrationTests extends AbstractConnectionIntegrationTests { - @Ignore("DATAREDIS-122 exec never returns null") - public void testWatch() throws Exception { - } - - @Ignore("DATAREDIS-122 exec never returns null") - public void testUnwatch() throws Exception { - } - @Test public void testMultiThreadsOneBlocking() throws Exception { Thread th = new Thread(new Runnable() { @@ -88,6 +81,7 @@ public class LettuceConnectionIntegrationTests extends AbstractConnectionIntegra assertEquals("delay", conn2.get("txs1")); } + @Ignore("DATAREDIS-189 Lettuce exec without multi times out waiting for a reply instead of throwing Exception") @Test public void testCloseInTransaction() { connection.multi(); diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionPipelineIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionPipelineIntegrationTests.java index 720f182eb..2b09efe70 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionPipelineIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionPipelineIntegrationTests.java @@ -59,17 +59,44 @@ public class LettuceConnectionPipelineIntegrationTests extends public void testMultiDiscard() { } - @Ignore("DATAREDIS-122 exec never returns null") - public void testWatch() throws Exception { - } - - @Ignore("DATAREDIS-122 exec never returns null") - public void testUnwatch() throws Exception { - } - // Overrides, usually due to return values being Long vs Boolean or Set vs // List + @Test + public void testWatch() throws Exception { + connection.set("testitnow", "willdo"); + connection.watch("testitnow".getBytes()); + //Give some time for watch to be asynch executed + Thread.sleep(500); + DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( + connectionFactory.getConnection()); + conn2.set("testitnow", "something"); + conn2.close(); + connection.multi(); + connection.set("testitnow", "somethingelse"); + actual.add(connection.exec()); + actual.add(connection.get("testitnow")); + verifyResults(Arrays.asList(new Object[] { null, "something" }), actual); + } + + @Test + public void testUnwatch() throws Exception { + connection.set("testitnow", "willdo"); + connection.watch("testitnow".getBytes()); + connection.unwatch(); + connection.multi(); + //Give some time for unwatch to be asynch executed + Thread.sleep(500); + DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( + connectionFactory.getConnection()); + conn2.set("testitnow", "something"); + connection.set("testitnow", "somethingelse"); + connection.get("testitnow"); + connection.exec(); + List convertedResults = convertResults(); + assertEquals(Arrays.asList(new Object[] {"somethingelse"}), convertedResults); + } + @Test public void testBLPop() { // Use a different synchronous connection to add items to the list, else blpop may diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionTransactionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionTransactionIntegrationTests.java index 26851fa87..16aabbcde 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionTransactionIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionTransactionIntegrationTests.java @@ -15,7 +15,8 @@ */ package org.springframework.data.redis.connection.lettuce; -import java.util.Arrays; +import static org.junit.Assert.assertNull; + import java.util.List; import org.junit.Ignore; @@ -87,10 +88,9 @@ public class LettuceConnectionTransactionIntegrationTests extends @Test public void exceptionExecuteNative() throws Exception { - actual.add(connection.execute("ZadD", getClass() + "#foo\t0.90\titem")); - // Syntax error on queued commands are swallowed and no results are - // returned - verifyResults(Arrays.asList(new Object[] {}), actual); + connection.execute("ZadD", getClass() + "#foo\t0.90\titem"); + // Syntax error on queued commands are swallowed and no results are returned + assertNull(getResults()); } protected void initConnection() { diff --git a/src/test/java/org/springframework/data/redis/connection/rjc/RjcConnectionPipelineIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/rjc/RjcConnectionPipelineIntegrationTests.java index 9acd85d90..2313b68f9 100644 --- a/src/test/java/org/springframework/data/redis/connection/rjc/RjcConnectionPipelineIntegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/rjc/RjcConnectionPipelineIntegrationTests.java @@ -170,6 +170,8 @@ public class RjcConnectionPipelineIntegrationTests extends public void testWatch() throws Exception { connection.set("testitnow", "willdo"); connection.watch("testitnow".getBytes()); + //Give some time for watch to be asynch executed + Thread.sleep(500); DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( connectionFactory.getConnection()); conn2.set("testitnow", "something"); @@ -188,6 +190,8 @@ public class RjcConnectionPipelineIntegrationTests extends connection.set("testitnow", "willdo"); connection.watch("testitnow".getBytes()); connection.unwatch(); + //Give some time for unwatch to be asynch executed + Thread.sleep(500); connection.multi(); DefaultStringRedisConnection conn2 = new DefaultStringRedisConnection( connectionFactory.getConnection()); diff --git a/src/test/java/org/springframework/data/redis/support/BoundKeyOperationsTest.java b/src/test/java/org/springframework/data/redis/support/BoundKeyOperationsTest.java index 106e05ff7..2b72a4188 100644 --- a/src/test/java/org/springframework/data/redis/support/BoundKeyOperationsTest.java +++ b/src/test/java/org/springframework/data/redis/support/BoundKeyOperationsTest.java @@ -31,7 +31,6 @@ import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; import org.springframework.data.redis.ConnectionFactoryTracker; import org.springframework.data.redis.connection.ConnectionUtils; -import org.springframework.data.redis.connection.jredis.JredisConnectionFactory; import org.springframework.data.redis.core.BoundKeyOperations; import org.springframework.data.redis.core.RedisTemplate; import org.springframework.data.redis.support.collections.ObjectFactory; @@ -70,6 +69,8 @@ public class BoundKeyOperationsTest { @Test public void testRename() throws Exception { + // DATAREDIS-188 Infinite loop renaming a non-existent Collection when using Lettuce + assumeTrue(!ConnectionUtils.isLettuce(template.getConnectionFactory())); Object key = keyOps.getKey(); assertNotNull(key); // RedisAtomicInteger/Long need to be reset, as they may be created diff --git a/src/test/java/org/springframework/data/redis/support/atomic/RedisAtomicTests.java b/src/test/java/org/springframework/data/redis/support/atomic/RedisAtomicTests.java index d0aac5989..c27f00022 100644 --- a/src/test/java/org/springframework/data/redis/support/atomic/RedisAtomicTests.java +++ b/src/test/java/org/springframework/data/redis/support/atomic/RedisAtomicTests.java @@ -151,9 +151,8 @@ public class RedisAtomicTests { // DATAREDIS-108 @Test public void testCompareSet() throws Exception { - // Txs not supported in Jredis, Lettuce checkAndSet not working DATAREDIS-122, - // SRP checkAndSet not working DATAREDIS-123 - assumeTrue(!ConnectionUtils.isJredis(factory) && !ConnectionUtils.isLettuce(factory) && !ConnectionUtils.isSrp(factory)); + // Txs not supported in Jredis, SRP checkAndSet not working DATAREDIS-123 + assumeTrue(!ConnectionUtils.isJredis(factory) && !ConnectionUtils.isSrp(factory)); final AtomicBoolean alreadySet = new AtomicBoolean(false); final int NUM = 50; final String KEY = getClass().getSimpleName() + ":atomic:counter";