From 7c8dafce672d6b5a7fe8e565e825cfa77878ba37 Mon Sep 17 00:00:00 2001 From: Jennifer Hickey Date: Thu, 30 May 2013 11:05:03 -0700 Subject: [PATCH] Redis timeouts should not be converted to 0 DATAREDIS-176 Only convert timeouts in Redis commands to 0 sec if the value to convert is 0, otherwise round up to 1 --- .../redis/core/DefaultListOperations.java | 8 +-- .../redis/core/DefaultValueOperations.java | 4 +- .../data/redis/core/RedisTemplate.java | 2 +- .../data/redis/core/TimeoutUtils.java | 49 +++++++++++++++ .../data/redis/core/TimeoutUtilsTests.java | 63 +++++++++++++++++++ 5 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 src/main/java/org/springframework/data/redis/core/TimeoutUtils.java create mode 100644 src/test/java/org/springframework/data/redis/core/TimeoutUtilsTests.java diff --git a/src/main/java/org/springframework/data/redis/core/DefaultListOperations.java b/src/main/java/org/springframework/data/redis/core/DefaultListOperations.java index 4f377da9e..41a6a39cf 100644 --- a/src/main/java/org/springframework/data/redis/core/DefaultListOperations.java +++ b/src/main/java/org/springframework/data/redis/core/DefaultListOperations.java @@ -52,11 +52,9 @@ class DefaultListOperations extends AbstractOperations implements Li } }, true); } - public V leftPop(K key, long timeout, TimeUnit unit) { - final int tm = (int) unit.toSeconds(timeout); - + final int tm = (int) TimeoutUtils.toSeconds(timeout, unit); return execute(new ValueDeserializingRedisCallback(key) { protected byte[] inRedis(byte[] rawKey, RedisConnection connection) { @@ -148,7 +146,7 @@ class DefaultListOperations extends AbstractOperations implements Li public V rightPop(K key, long timeout, TimeUnit unit) { - final int tm = (int) unit.toSeconds(timeout); + final int tm = (int) TimeoutUtils.toSeconds(timeout, unit); return execute(new ValueDeserializingRedisCallback(key) { @@ -211,7 +209,7 @@ class DefaultListOperations extends AbstractOperations implements Li public V rightPopAndLeftPush(K sourceKey, K destinationKey, long timeout, TimeUnit unit) { - final int tm = (int) unit.toSeconds(timeout); + final int tm = (int)TimeoutUtils.toSeconds(timeout, unit); final byte[] rawDestKey = rawKey(destinationKey); return execute(new ValueDeserializingRedisCallback(sourceKey) { diff --git a/src/main/java/org/springframework/data/redis/core/DefaultValueOperations.java b/src/main/java/org/springframework/data/redis/core/DefaultValueOperations.java index a41877124..57ea74272 100644 --- a/src/main/java/org/springframework/data/redis/core/DefaultValueOperations.java +++ b/src/main/java/org/springframework/data/redis/core/DefaultValueOperations.java @@ -185,12 +185,12 @@ class DefaultValueOperations extends AbstractOperations implements V public void set(K key, V value, long timeout, TimeUnit unit) { final byte[] rawKey = rawKey(key); final byte[] rawValue = rawValue(value); - final long rawTimeout = unit.toSeconds(timeout); + final long rawTimeout = TimeoutUtils.toSeconds(timeout, unit); execute(new RedisCallback() { public Object doInRedis(RedisConnection connection) throws DataAccessException { - connection.setEx(rawKey, (int) rawTimeout, rawValue); + connection.setEx(rawKey, rawTimeout, rawValue); return null; } }, true); 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 6354d2277..5a8a04617 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisTemplate.java +++ b/src/main/java/org/springframework/data/redis/core/RedisTemplate.java @@ -472,7 +472,7 @@ public class RedisTemplate extends RedisAccessor implements RedisOperation public Boolean expire(K key, long timeout, TimeUnit unit) { final byte[] rawKey = rawKey(key); - final long rawTimeout = unit.toSeconds(timeout); + final long rawTimeout = TimeoutUtils.toSeconds(timeout, unit); return execute(new RedisCallback() { diff --git a/src/main/java/org/springframework/data/redis/core/TimeoutUtils.java b/src/main/java/org/springframework/data/redis/core/TimeoutUtils.java new file mode 100644 index 000000000..caa47ddd7 --- /dev/null +++ b/src/main/java/org/springframework/data/redis/core/TimeoutUtils.java @@ -0,0 +1,49 @@ +/* + * Copyright 2013 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.core; + +import java.util.concurrent.TimeUnit; + +/** + * Helper class featuring methods for calculating Redis timeouts + * + * @author Jennifer Hickey + */ +abstract public class TimeoutUtils { + + /** + * Converts the given timeout to seconds. + *

+ * Since a 0 timeout blocks some Redis ops indefinitely, this method will + * return 1 if the original value is greater than 0 but is truncated to 0 on + * conversion. + * + * @param timeout + * The timeout to convert + * @param unit + * The timeout's unit + * @return The converted timeout + */ + public static long toSeconds(long timeout, TimeUnit unit) { + long seconds = unit.toSeconds(timeout); + // A 0 timeout blocks some Redis ops indefinitely, round up if that's + // not the intention + if (timeout > 0 && seconds == 0) { + seconds = 1; + } + return seconds; + } +} \ No newline at end of file diff --git a/src/test/java/org/springframework/data/redis/core/TimeoutUtilsTests.java b/src/test/java/org/springframework/data/redis/core/TimeoutUtilsTests.java new file mode 100644 index 000000000..da05d7e37 --- /dev/null +++ b/src/test/java/org/springframework/data/redis/core/TimeoutUtilsTests.java @@ -0,0 +1,63 @@ +/* + * Copyright 2013 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.core; + +import java.util.concurrent.TimeUnit; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +/** + * Unit test of {@link TimeoutUtils} + * + * @author Jennifer Hickey + * + */ +public class TimeoutUtilsTests { + + @Test + public void testConvertMoreThanOneSecond() { + assertEquals(2, TimeoutUtils.toSeconds(2010, TimeUnit.MILLISECONDS)); + } + + @Test + public void testConvertLessThanOneSecond() { + assertEquals(1, TimeoutUtils.toSeconds(999, TimeUnit.NANOSECONDS)); + } + + @Test + public void testConvertZero() { + assertEquals(0, TimeoutUtils.toSeconds(0, TimeUnit.MINUTES)); + } + + @Test + public void testConvertNegativeGreaterThanNegativeOne() { + // Ensure we convert this to 0 as before, though ideally we wouldn't accept negative values + assertEquals(0,TimeoutUtils.toSeconds(-123, TimeUnit.MILLISECONDS)); + } + + @Test + public void testConvertNegativeEqualNegativeOne() { + assertEquals(-1,TimeoutUtils.toSeconds(-1111, TimeUnit.MILLISECONDS)); + } + + @Test + public void testConvertNegativeLessThanNegativeOne() { + assertEquals(-2,TimeoutUtils.toSeconds(-2344, TimeUnit.MILLISECONDS)); + } + +}