312531 - ConnectionPool.getConnFromPool and ConnectionPool.close synchronized method

Status: RESOLVED WONTFIX

(Directory Graveyard :: LDAP Java SDK, defect)

Description

[Pirasenna Thiyagarajan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414

ConnectionPool object does not perform in a enterprise environment

Reproducible: Always

Steps to Reproduce:
1. Create connection pool of size 500.
2. Access directory records with  50, 100, 150, 200, 250 threads with the
following steps.
2a. Get connection from connection pool (ConnectionPool.getConnection)
2b. Perform directory entry lookup.
2c. return connection to connection pool using ConnectionPool.close.

Actual Results:  
You'll find very few threads actually using the connection and a lot of threads
actually blocked by the two synchronized methods ConnectionPool.getConnFromPool
and ConnectionPool.close.

Expected Results:  
The methods should never be synchronized.  This blocks entry to the entire
object causing global contention.  Instead the lock must be acquired in a real
critical section.

Comment 1

[Pirasenna Thiyagarajan]

Attachment: Modified implementation that reduces synchronization.

Comment 2

[Pirasenna Thiyagarajan]

Attachment: The diff between 4.17 source file and my implementation

The implementation removes method level synchronization and adds
synchronization in critical sections.
This code has been tested in extreme hi-test use environment.

Comment 3

[Pirasenna Thiyagarajan]

Attachment: Updated ConnectionPool class, which is a little better than my previous implementation.

This is the updated code that fixes the synchronization problem.

Comment 4

[Pirasenna Thiyagarajan]

Comment on attachment 199669 [details] [diff] [review]
The diff between 4.17 source file and my implementation

--- ConnectionPool.java 2005-10-16 15:01:35.134625000 -0700
+++
\workspace\ldapjdk2\mozilla\directory\java-sdk\ldapjdk\netscape\ldap\util\Conne
ctionPool.java	     2002-12-02 15:35:54.000000000 -0800
@@ -172,19 +172,19 @@ public class ConnectionPool {
     }

     /**
      * Destroy the whole pool - called during a shutdown
      */
     public void destroy() {
	 for ( int i = 0; i < pool.size(); i++ ) {
	     disconnect(
-		 (LDAPConnectionObject)pool.get(i) );
+		 (LDAPConnectionObject)pool.elementAt(i) );
	 }
-	 pool.clear();
+	 pool.removeAllElements();
     }

     /**
      * Gets a connection from the pool
      *
      * If no connections are available, the pool will be
      * extended if the number of connections is less than
      * the maximum; if the pool cannot be extended, the method
@@ -247,81 +247,76 @@ public class ConnectionPool {
      *
      * If no connections are available, the pool will be
      * extended if the number of connections is less than
      * the maximum; if the pool cannot be extended, the method
      * returns null.
      *
      * @return an active connection or null.
      */
-    protected LDAPConnection getConnFromPool() {
+    protected synchronized LDAPConnection getConnFromPool() {
	 LDAPConnection con = null;
	 LDAPConnectionObject ldapconnobj = null;

	 int pSize = pool.size();

	 // Get an available connection
	 for ( int i = 0; i < pSize; i++ ) {

	     // Get the ConnectionObject from the pool
	     LDAPConnectionObject co = 
-		 (LDAPConnectionObject)pool.get(i);
-	     synchronized(co) {
-		    if ( co.isAvailable() ) {  // Conn available?
-			ldapconnobj = co;
-			co.setInUse(true);
-			break;
-		    }
+		 (LDAPConnectionObject)pool.elementAt(i);
+
+	     if ( co.isAvailable() ) {	// Conn available?
+		 ldapconnobj = co;
+		 break;
	     }
	 }

-	 if ( ldapconnobj == null && pool.size() < poolMax) {
+	 if ( ldapconnobj == null ) {
	     // If there there were no conns in pool, can we grow
	     // the pool?
-		synchronized(pool) {
-			pSize = pool.size();
-
-		    if ( (poolMax < 0) ||
-			 ( (poolMax > 0) &&
-			   (pSize < poolMax)) ) {
-	
-			// Yes we can grow it
-			ldapconnobj = addConnection();
-	
-			// If a new connection was created, use it
-			if ( ldapconnobj != null ) {
-				ldapconnobj.setInUse(true);
-				pool.add(ldapconnobj);
-			}
-		    } else {
-			debug("All pool connections in use");
-		    }
-		}
+	     if ( (poolMax < 0) ||
+		  ( (poolMax > 0) &&
+		    (pSize < poolMax)) ) {
+
+		 // Yes we can grow it
+		 int i = addConnection();
+
+		 // If a new connection was created, use it
+		 if ( i >= 0 ) {
+		     ldapconnobj = 
+			 (LDAPConnectionObject)pool.elementAt(i);
+		 }
+	     } else {
+		 debug("All pool connections in use");
+	     }
	 }

	 if ( ldapconnobj != null ) {
+	     ldapconnobj.setInUse( true );  // Mark as in use
	     con = ldapconnobj.getLDAPConn();
	 }
	 return con;
     }

     /**
      * This is our soft close - all we do is mark
      * the connection as available for others to use.
      * We also reset the auth credentials in case
      * they were changed by the caller.
      *
      * @param ld a connection to return to the pool
      */
-    public void close( LDAPConnection ld ) {
+    public synchronized void close( LDAPConnection ld ) {

	 int index = find( ld );
	 if ( index != -1 ) {
	     LDAPConnectionObject co = 
-		 (LDAPConnectionObject)pool.get(index);
+		 (LDAPConnectionObject)pool.elementAt(index);

	     // Reset the auth if necessary
	     if (ldc == null || !ldc.getAuthenticationMethod().equals("sasl"))
{

		 boolean reauth = false;
		 //if user bound anon then getAuthenticationDN is null
		 if ( ld.getAuthenticationDN() == null ) {
		       reauth = (authdn != null); 
@@ -349,17 +344,17 @@ public class ConnectionPool {

     /**
      * Debug method to print the contents of the pool
      */
     public void printPool(){
	 System.out.println("--ConnectionPool--");
	 for ( int i = 0; i < pool.size(); i++ ) {
	     LDAPConnectionObject co =
-		 (LDAPConnectionObject)pool.get(i);
+		 (LDAPConnectionObject)pool.elementAt(i);
	     System.out.println( "" + i + "=" + co );
	 }
     }

     private void disconnect(
	 LDAPConnectionObject ldapconnObject ) {
	 if ( ldapconnObject != null ) {
	     if (ldapconnObject.isAvailable()) {  
@@ -388,86 +383,86 @@ public class ConnectionPool {
	 }

	 debug("****Initializing LDAP Pool****");
	 debug("LDAP host = "+host+" on port "+port);
	 debug("Number of connections="+poolSize);
	 debug("Maximum number of connections="+poolMax);
	 debug("******");

-	 pool = new ArrayList(); // Create pool vector
+	 pool = new java.util.Vector(); // Create pool vector
	 setUpPool( poolSize ); // Initialize it
     }

-    private LDAPConnectionObject addConnection() {
-	 LDAPConnectionObject ldapconnobj = null;
+    private int addConnection() {
+	 int index = -1;

	 debug("adding a connection to pool...");
	 try {
-	     ldapconnobj = createConnection();
+	     int size = pool.size() + 1; // Add one connection
+	     setUpPool( size );
+
+	     if ( size == pool.size() ) {
+		 // New size is size requested?
+		 index = size - 1;
+	     }
	 } catch (Exception ex) {
-	     debug("Error while adding a connection: "+ex.toString());
+	     debug("Adding a connection: "+ex.toString());
	 }
-	 return ldapconnobj;
+	 return index;
     }

-    private void setUpPool( int size )
+    private synchronized void setUpPool( int size )
	 throws LDAPException {
-	synchronized(pool) {
-		// Loop on creating connections
-		while( pool.size() < size ) {
-		    pool.add(createConnection());
-		}
-	}
-    }
-
-    private LDAPConnectionObject createConnection() throws LDAPException {
-	 LDAPConnectionObject co =
-	     new LDAPConnectionObject();
-	 // Make LDAP connection, using template if available
-	 LDAPConnection newConn =
-	     (ldc != null) ? (LDAPConnection)ldc.clone() :
-	     new LDAPConnection();
-	 co.setLDAPConn(newConn);
-	 try {
-	     if ( newConn.isConnected() ) {
-		 // If using a template, then reconnect
-		 // to create a separate physical connection
-		 newConn.reconnect();
-	     } else {
-		 // Not using a template, so connect with
-		 // simple authentication using ldap v3
-		 try { 
-		     newConn.connect( 3, host, port, authdn, authpw); 
-		 }
-		 catch (LDAPException connEx) {
-		     // fallback to ldap v2 if v3 is not supported
-		     if (connEx.getLDAPResultCode() ==
LDAPException.PROTOCOL_ERROR) {
-			 newConn.connect( 2, host, port, authdn, authpw); 
+	 // Loop on creating connections
+	 while( pool.size() < size ) {
+	     LDAPConnectionObject co =
+		 new LDAPConnectionObject();
+	     // Make LDAP connection, using template if available
+	     LDAPConnection newConn =
+		 (ldc != null) ? (LDAPConnection)ldc.clone() :
+		 new LDAPConnection();
+	     co.setLDAPConn(newConn);
+	     try {
+		 if ( newConn.isConnected() ) {
+		     // If using a template, then reconnect
+		     // to create a separate physical connection
+		     newConn.reconnect();
+		 } else {
+		     // Not using a template, so connect with
+		     // simple authentication using ldap v3
+		     try { 
+			 newConn.connect( 3, host, port, authdn, authpw); 
		     }
-		     else {
-			 throw connEx;
+		     catch (LDAPException connEx) {
+			 // fallback to ldap v2 if v3 is not supported
+			 if (connEx.getLDAPResultCode() ==
connEx.PROTOCOL_ERROR) {
+			     newConn.connect( 2, host, port, authdn, authpw); 
+			 }
+			 else {
+			     throw connEx;
+			 }
		     }
		 }
+	     } catch ( LDAPException le ) {
+		 debug("Creating pool:"+le.toString());
+		 debug("aborting....");
+		 throw le;
	     }
-	 } catch ( LDAPException le ) {
-	     debug("Creating pool:"+le.toString());
-	     debug("aborting....");
-	     throw le;
+	     co.setInUse( false ); // Mark not in use
+	     pool.addElement( co );
	 }
-	 co.setInUse( false ); // Mark not in use
-		return co;
     }

     private int find( LDAPConnection con ) {
	 // Find the matching Connection in the pool
	 if ( con != null ) {
	     for ( int i = 0; i < pool.size(); i++ ) {
		 LDAPConnectionObject co = 
-		     (LDAPConnectionObject)pool.get(i);
+		     (LDAPConnectionObject)pool.elementAt(i);
		 if ( co.getLDAPConn() == con ) {
		     return i;
		 }
	     }
	 }
	 return -1;
     }

@@ -502,20 +497,16 @@ public class ConnectionPool {
	 }
     }

     /**
      * Wrapper for LDAPConnection object in pool
      */
     class LDAPConnectionObject{

-	LDAPConnectionObject() {
-		inUse = false;
-	}
-
	 /**
	  * Returns the associated LDAPConnection.
	  *
	  * @return the LDAPConnection.
	  * 
	  */
	 LDAPConnection getLDAPConn() {
	     return this.ld;
@@ -566,11 +557,11 @@ public class ConnectionPool {

     private int poolSize; // Min pool size
     private int poolMax; // Max pool size
     private String host; // LDAP host
     private int port;	  // Port to connect at
     private String authdn;  // Identity of connections
     private String authpw;  // Password for authdn
     private LDAPConnection ldc = null; // Connection to clone
-    private java.util.ArrayList pool;	// the actual pool
+    private java.util.Vector pool;  // the actual pool
     private boolean debugMode;
 }

Comment 5

[Pirasenna Thiyagarajan]

Attachment: New Diffs for the updated changes

This diff adds one check before getting into the synchronization block in
getConnFromPool

Comment 6

[Mark Smith [:mcs]]

I am not familiar with the Java LDAP SDK connection pool code, but I doubt the
original implementors envisioned using a pool size near 500 or more than 10-20
threads.  Before we land these changes, we need review from some people with
more Java expertise than myself.  It would also be good to know if the latest
proposed patch is well tested.

Comment 7

[Pirasenna Thiyagarajan]

Even with 20-30 threadds, the contention for lock on ConnectionPool object while
entering getConnFromPool and close will still occur.

These changes were tested with 500 concurrent threads retrieveing LDAPConnection
performing basic search operations and returning them.

Comment 8

[Pirasenna Thiyagarajan]

(In reply to comment #7)
> Even with 20-30 threadds, the contention for lock on ConnectionPool object while
> entering getConnFromPool and close will still occur.
I meant, even with 20-30 connections in the pool, as number of threads increase,
contention will increase.
> 
> These changes were tested with 500 concurrent threads retrieveing LDAPConnection
> performing basic search operations and returning them.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Not activity for at least 6 years, closing.