Skip to content
Browse files

Fix severe bug in connection pool. It turns out commons pool synchron…

…izes the entire makeObject() call. This means that only one connection to ANY server can be connecting at a time. This problems shows up quite severely when you have a host that is hard down on the network, and hence the connection will timeout. Exacerbating this it turns out that java.net.Socket does not use the soTimeout when establishing the connection, so we were getting a near infinite timeout which blocked all other connections from being established.

Fix is to use soTimeout when establishing a connection, and also upgrade to new version of commons pool which seems to have fixed (or mostly fixed) this problem.
  • Loading branch information...
1 parent 3a3db2b commit 95ab1e2340c68e99f03044d831f388f9d8f2420a @jkreps jkreps committed Jun 21, 2009
View
2 .classpath
@@ -15,7 +15,6 @@
<classpathentry kind="lib" path="lib/commons-codec-1.3.jar"/>
<classpathentry kind="lib" path="lib/commons-dbcp-1.2.2.jar"/>
<classpathentry kind="lib" path="lib/commons-httpclient-3.1.jar"/>
- <classpathentry kind="lib" path="lib/commons-pool-1.4.jar"/>
<classpathentry kind="lib" path="lib/colt-1.2.0.jar"/>
<classpathentry kind="lib" path="lib/libthrift-20080411p1.jar"/>
<classpathentry kind="lib" path="lib/google-collect-snapshot-20090211.jar"/>
@@ -39,5 +38,6 @@
<classpathentry kind="lib" path="lib/servlet-api-2.5.jar"/>
<classpathentry kind="lib" path="lib/xercesImpl-2.9.1.jar"/>
<classpathentry kind="lib" path="lib/commons-logging-1.1.1.jar"/>
+ <classpathentry kind="lib" path="lib/commons-pool-1.5.1.jar"/>
<classpathentry kind="output" path="classes"/>
</classpath>
View
BIN lib/commons-pool-1.4.jar → lib/commons-pool-1.5.1.jar
Binary file not shown.
View
4 src/java/voldemort/server/socket/SocketServerSession.java
@@ -79,11 +79,13 @@ public void run() {
} catch(EOFException e) {
logger.info("Client " + socket.getRemoteSocketAddress() + " disconnected.");
} catch(IOException e) {
+ // if this is an unexpected
if(!isClosed)
logger.error(e);
} finally {
try {
- socket.close();
+ if(!socket.isClosed())
+ socket.close();
} catch(Exception e) {
logger.error("Error while closing socket", e);
}
View
6 src/java/voldemort/store/routed/RoutedStore.java
@@ -51,6 +51,7 @@
import voldemort.store.StoreUtils;
import voldemort.store.UnreachableStoreException;
import voldemort.utils.ByteArray;
+import voldemort.utils.ByteUtils;
import voldemort.utils.SystemTime;
import voldemort.utils.Time;
import voldemort.utils.Utils;
@@ -222,7 +223,7 @@ public void run() {
boolean acquired = semaphore.tryAcquire(timeoutMs, TimeUnit.MILLISECONDS);
if(!acquired)
logger.warn("Delete operation timed out waiting for operation " + i
- + "to complete after waiting " + timeoutMs + " ms.");
+ + " to complete after waiting " + timeoutMs + " ms.");
// okay, at least the required number of operations have
// completed, were they successful?
if(successes.get() >= attempts)
@@ -450,6 +451,9 @@ public void run() {
public void run() {
try {
+ if(logger.isTraceEnabled())
+ logger.trace("Attempting get operation on node " + node.getId()
+ + " for key '" + ByteUtils.toHexString(key.get()) + "'.");
List<Versioned<byte[]>> fetched = innerStores.get(node.getId()).get(key);
retrieved.addAll(fetched);
if(repairReads) {
View
2 src/java/voldemort/store/socket/SocketPoolableObjectFactory.java
@@ -84,7 +84,7 @@ public Object makeObject(Object key) throws Exception {
socket.setSendBufferSize(this.socketBufferSize);
socket.setTcpNoDelay(true);
socket.setSoTimeout(soTimeoutMs);
- socket.connect(new InetSocketAddress(dest.getHost(), dest.getPort()));
+ socket.connect(new InetSocketAddress(dest.getHost(), dest.getPort()), soTimeoutMs);
recordSocketCreation(dest, socket);
View
67 test/integration/voldemort/CommonsPoolTest.java
@@ -0,0 +1,67 @@
+package voldemort;
+
+import java.util.Date;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.commons.pool.KeyedObjectPool;
+import org.apache.commons.pool.KeyedPoolableObjectFactory;
+import org.apache.commons.pool.impl.GenericKeyedObjectPool;
+
+public class CommonsPoolTest {
+
+ public static void main(String[] args) {
+ GenericKeyedObjectPool.Config config = new GenericKeyedObjectPool.Config();
+ config.maxActive = 100;
+ config.maxTotal = 100;
+ config.maxIdle = 100;
+ config.maxWait = 1000000;
+ config.testOnBorrow = true;
+ config.testOnReturn = true;
+ config.whenExhaustedAction = GenericKeyedObjectPool.WHEN_EXHAUSTED_BLOCK;
+ KeyedPoolableObjectFactory objFactory = new TestPoolableObjectFactory();
+ final KeyedObjectPool pool = new GenericKeyedObjectPool(objFactory, config);
+ ExecutorService executor = Executors.newFixedThreadPool(20);
+ System.out.println("You should now see 20 quick checkouts in rapid succession:");
+ for(int i = 0; i < 20; i++) {
+ final String key = Integer.toString(i);
+ executor.execute(new Runnable() {
+
+ public void run() {
+ try {
+ System.out.println("Trying to borrow " + key + " at " + new Date());
+ pool.borrowObject(key);
+ System.out.println("Borrowing of " + key + " completed at " + new Date());
+ } catch(Exception e) {
+ e.printStackTrace();
+ }
+ }
+ });
+ }
+ }
+
+ private static class TestPoolableObjectFactory implements KeyedPoolableObjectFactory {
+
+ private AtomicBoolean flag = new AtomicBoolean(false);
+
+ public void activateObject(Object k, Object v) throws Exception {}
+
+ public void destroyObject(Object k, Object v) throws Exception {}
+
+ public Object makeObject(Object k) throws Exception {
+ boolean isFirst = flag.compareAndSet(false, true);
+ if(isFirst)
+ Thread.sleep(20 * 1000);
+ return new Object();
+ }
+
+ public void passivateObject(Object k, Object v) throws Exception {}
+
+ public boolean validateObject(Object k, Object v) {
+ return true;
+ }
+
+ }
+
+}

0 comments on commit 95ab1e2

Please sign in to comment.
Something went wrong with that request. Please try again.