Connection pool implementation

Question

I wanted to try out writing an minimalistic connection pool implementation (out of curiosity and for learning). Could you all please review the code and provide feedback?

There are two classes (class names are hyperlinked to code):

1. com.amitcodes.dbcp.ConnectionPool: The connection pool implementation
2. com.amitcodes.dbcp.PooledConnection: The proxy for java.sql.Connection, written with the intent of ensuring that connections borrowed from ConnectionPool should be not be closed by client code, but surrendered back to the pool.

---

TODOs (based on review comments):

This is why code reviews are important. I've added the following changes so far based on the code review comments:

1. maintain a count of active connections available in the pool (preferably using AtomicInteger)
2. borrowConnection() should ensure there are no available idle connections (using point #1 above) before opening a new pooled connection
3. surrenderConnection should rollback unfinished transactions. They could leak.
4. validate that surrendered connections are same as ones which were borrowed. If this check is not in place, a client can surrender connection to db 'foo' to dbcp for db 'bar'
5. surrenderConnection() should take care of rolling back any open transactions
6. include a validate() method to validate constructor params

---

The code:

package com.amitcodes.dbcp;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.logging.Level;
import java.util.logging.Logger;
public class ConnectionPool {
 private static final Logger logger = Logger.getLogger(ConnectionPool.class.getCanonicalName());
 private BlockingQueue<Connection> pool;
 /**
 * Maximum number of connections that the pool can have
 */
 private int maxPoolSize;
 /**
 * Number of connections that should be created initially
 */
 private int initialPoolSize;
 /**
 * Number of connections generated so far
 */
 private int currentPoolSize;
 private String dbUrl;
 private String dbUser;
 private String dbPassword;
 public ConnectionPool(int maxPoolSize, int initialPoolSize, String url, String username,
 String password, String driverClassName) throws ClassNotFoundException, SQLException {
 if ((initialPoolSize > maxPoolSize) || initialPoolSize < 1 || maxPoolSize < 1) {
 throw new IllegalArgumentException("Invalid pool size parameters");
 }
 // default max pool size to 10
 this.maxPoolSize = maxPoolSize > 0 ? maxPoolSize : 10;
 this.initialPoolSize = initialPoolSize;
 this.dbUrl = url;
 this.dbUser = username;
 this.dbPassword = password;
 this.pool = new LinkedBlockingQueue<Connection>(maxPoolSize);
 initPooledConnections(driverClassName);
 if (pool.size() != initialPoolSize) {
 logger.log(Level.WARNING,
 "Initial sized pool creation failed. InitializedPoolSize={0}, initialPoolSize={1}",
 new Object[]{pool.size(), initialPoolSize});
 }
 }
 private void initPooledConnections(String driverClassName)
 throws ClassNotFoundException, SQLException {
 // 1. Attempt to load the driver class
 Class.forName(driverClassName);
 // 2. Create and pool connections
 for (int i = 0; i < initialPoolSize; i++) {
 openAndPoolConnection();
 }
 }
 private synchronized void openAndPoolConnection() throws SQLException {
 if (currentPoolSize == maxPoolSize) {
 return;
 }
 Connection conn = DriverManager.getConnection(dbUrl, dbUser, dbPassword);
 pool.offer(new PooledConnection(conn, this));
 currentPoolSize++;
 logger.log(Level.FINE, "Created connection {0}, currentPoolSize={1}, maxPoolSize={2}",
 new Object[]{conn, currentPoolSize, maxPoolSize});
 }
 public Connection borrowConnection() throws InterruptedException, SQLException {
 if (pool.peek()==null && currentPoolSize < maxPoolSize) {
 openAndPoolConnection();
 }
 // Borrowing thread will be blocked till connection
 // becomes available in the queue
 return pool.take();
 }
 public void surrenderConnection(Connection conn) {
 if (!(conn instanceof PooledConnection)) {
 return;
 }
 pool.offer(conn); // offer() as we do not want to go beyond capacity
 }
}

com.amitcodes.dbcp.PooledConnection: Only relevant section is posted here and boiler-plate code has been removed. You can view the complete class here on GitHub.

package com.amitcodes.dbcp;
import java.sql.*;
import java.util.*;
import java.util.concurrent.Executor;
public class PooledConnection implements Connection {
 private Connection coreConnection;
 private ConnectionPool connectionPool;
 public PooledConnection(Connection coreConnection, ConnectionPool connectionPool) {
 this.connectionPool = connectionPool;
 this.coreConnection = coreConnection;
 }
 @Override
 public void close() throws SQLException {
 connectionPool.surrenderConnection(this);
 }
 /* ****************************************************************
 * Proxy Methods
 * ****************************************************************/
 @Override
 public Statement createStatement() throws SQLException {
 return coreConnection.createStatement();
 }
 @Override
 public PreparedStatement prepareStatement(String sql) throws SQLException {
 return coreConnection.prepareStatement(sql);
 }
 // SOME CODE SKIPPED
}

• Aniket Thakur: Lets say a client calls surrenderConnection(connection) but still keeps a reference to the conenction he can still go ahead and call methods like createStatement() and prepareStatement() even though the connection is returned to the poll that might me handed over to another client? How will we resolve this?

Answer 1

A few random notes:

1. If I were you I'd check the source and API of Apache Commons Pool. You could borrow ideas about possible errors, corner cases, useful API calls, parameters etc.

2. surrenderConnection should rollback unfinished transactions. They could leak.

3. SLF4J has better API than JUL. (Check Logback also, if you don't familiar with it.)

4. A malicious (or poorly written client with multiple pools) can call surrenderConnection with a PooledConnection instance which was not created by the called ConnectionPool.

5. I'd check nulls in the constructors/methods. Does it make sense to call them with null? If not, check it and throw an exception. checkNotNull in Guava is a great choice for that.

   this.connectionPool = 
    checkNotNull(connectionPool, "connectionPool cannot be null");
   

   (Also see: Effective Java, 2nd edition, Item 38: Check parameters for validity)

• Amit Sharma: Awesome :) Point #2 and #4 would have caused serious bugs. Will fix 'em and update the code soon.
• Archimedes Trajano: I generally avoid other logging apis like SLF4J and use JUL to avoid additional library dependencies. Java EE servers also use JUL to capture and manage the logs as well. If you use a logging API then it becomes and application responsibility.

Answer 2

It looks like the borrow connection code will create a new connection without first checking to see if there is an available connection in the pool. That's not how most connection pools work. You probably want to store a count of available connections (consider using an AtomicInteger for safe concurrent access) and check against that before adding the new connection.

Other than that it looks pretty solid. However, it looks like your test coverage is pretty thin. I suggest beefing that up. Unit tests are an excellent way to make sure your code performs as expected.

• Amit Sharma: I pushed in a quick fix to peek the queue before opening a new pooled-connection (in the inline code). Will give some more thought on using AtomicInteger and update the code. Thanks for the review :)

Answer 3

I didn't understand the concept here. How come ConnectionPool is a part of PooledConnection?

My understanding of a pool - you a bucket with 100 items, take one, use it and put it back into the pond.

I would prefer writing something like:

ConnectionPool.getConnection();
ConnectionPool.releaseConnection(Connection c);

Where there will be only 1 instance of this pool. (I would use a private constructor and a getInstance() for pool)

• avmohan: Making ConnectionPool a singleton (private constructor & singleton) is a bad idea. What if same client wants to have 2 connection pools for connecting to 2 urls?
• Ram B Gorre: In that case, you should take 2 connections from the same pool. You should not create multiple pools.
• avmohan: If an app needs to connect to both db1 & db2, the connections cannot be in the same pool. The connections are not interchangeable
• Ram B Gorre: OMG. dude, you didn't get it. A pool is for one db. For a different db, there should be a different pool. In such cases, you may need a pool factory to handover pool objects to you when you pass db info.
• avmohan: What I'm saying is - making it a true Singleton - with private constructor and all makes it impossible to make different pools per db. So the better option is to keep it non-jvm-singleton. It can be instantiated through a factory or a DI framework