From 3257827f01f7b15d6e8c7b3bbd6f05e2caebff5a Mon Sep 17 00:00:00 2001 From: Tobias Ara Svensson Date: Mon, 24 Nov 2014 12:23:42 +0100 Subject: [PATCH] Fixes #791 Exceptions in close cause empty JedisPool and then deadlock --- .../java/redis/clients/jedis/JedisPool.java | 16 ++++-- .../clients/jedis/tests/JedisPoolTest.java | 55 +++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/main/java/redis/clients/jedis/JedisPool.java b/src/main/java/redis/clients/jedis/JedisPool.java index fea6177..c75390c 100644 --- a/src/main/java/redis/clients/jedis/JedisPool.java +++ b/src/main/java/redis/clients/jedis/JedisPool.java @@ -5,6 +5,7 @@ import java.net.URI; import org.apache.commons.pool2.impl.GenericObjectPool; import org.apache.commons.pool2.impl.GenericObjectPoolConfig; +import redis.clients.jedis.exceptions.JedisException; import redis.clients.util.JedisURIHelper; import redis.clients.util.Pool; @@ -112,10 +113,17 @@ public class JedisPool extends Pool { } public void returnResource(final Jedis resource) { - if (resource != null) { - resource.resetState(); - returnResourceObject(resource); - } + if (resource != null) { + try { + resource.resetState(); + returnResourceObject(resource); + } + catch (Exception e) { + returnBrokenResource(resource); + throw new JedisException( + "Could not return the resource to the pool", e); + } + } } public int getNumActive() { diff --git a/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java b/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java index ef8e80e..5a6fc6c 100644 --- a/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java +++ b/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java @@ -1,8 +1,13 @@ package redis.clients.jedis.tests; +import java.net.SocketTimeoutException; import java.net.URI; import java.net.URISyntaxException; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.pool2.PooledObject; +import org.apache.commons.pool2.PooledObjectFactory; +import org.apache.commons.pool2.impl.DefaultPooledObject; import org.apache.commons.pool2.impl.GenericObjectPoolConfig; import org.junit.Assert; import org.junit.Test; @@ -206,6 +211,56 @@ public class JedisPoolTest extends Assert { assertTrue(pool0.isClosed()); } + @Test + public void returnResourceDestroysResourceOnException() { + + class CrashingJedis extends Jedis { + @Override + public void resetState() { + throw new RuntimeException(); + } + } + + final AtomicInteger destroyed = new AtomicInteger(0); + + class CrashingJedisPooledObjectFactory implements PooledObjectFactory { + + @Override + public PooledObject makeObject() throws Exception { + return new DefaultPooledObject(new CrashingJedis()); + } + + @Override + public void destroyObject(PooledObject p) throws Exception { + destroyed.incrementAndGet(); + } + + @Override + public boolean validateObject(PooledObject p) { + return true; + } + + @Override + public void activateObject(PooledObject p) throws Exception {} + + @Override + public void passivateObject(PooledObject p) throws Exception {} + } + + GenericObjectPoolConfig config = new GenericObjectPoolConfig(); + config.setMaxTotal(1); + JedisPool pool = new JedisPool(config, hnp.getHost(), hnp.getPort(), 2000, "foobared"); + pool.initPool(config, new CrashingJedisPooledObjectFactory()); + Jedis crashingJedis = pool.getResource(); + + try { + pool.returnResource(crashingJedis); + } + catch (Exception ignored) {} + + assertEquals(destroyed.get(), 1); + } + @Test public void returnResourceShouldResetState() { GenericObjectPoolConfig config = new GenericObjectPoolConfig();