Am I Using JDBC Connection Pooling?

Question

I am trying to determine if I am actually using JDBC connection pooling. After doing some research, the implementation almost seems too easy. Easier than a regular connection in fact so i'd like to verify.

Here is my connection class:

public class DatabaseConnection {

Connection conn = null;

public Connection getConnection() {

    BasicDataSource bds = new BasicDataSource();
    bds.setDriverClassName("com.mysql.jdbc.Driver");
    bds.setUrl("jdbc:mysql://localhost:3306/data");
    bds.setUsername("USERNAME");
    bds.setPassword("PASSWORD");

    try{
        System.out.println("Attempting Database Connection");
        conn = bds.getConnection();
        System.out.println("Connected Successfully");
    }catch(SQLException e){
        System.out.println("Caught SQL Exception: " + e);
    }
    return conn;
}

public void closeConnection() throws SQLException {
    conn.close();
}

}

Is this true connection pooling? I am using the connection in another class as so:

        //Check data against database.
    DatabaseConnection dbConn = new DatabaseConnection();
    Connection conn;
    ResultSet rs;
    PreparedStatement prepStmt;

    //Query database and check username/pass against table.
    try{
        conn = dbConn.getConnection();
        String sql = "SELECT * FROM users WHERE username=? AND password=?";
        prepStmt = conn.prepareStatement(sql);
        prepStmt.setString(1, user.getUsername());
        prepStmt.setString(2, user.getPassword());
        rs = prepStmt.executeQuery();

        if(rs.next()){ //Found Match.
            do{
                out.println("UserName = " + rs.getObject("username") + " Password = " + rs.getObject("password"));
                out.println("<br>");
            } while(rs.next());
        } else {
            out.println("Sorry, you are not in my database."); //No Match.
        }

        dbConn.closeConnection(); //Close db connection.

    }catch(SQLException e){
        System.out.println("Caught SQL Exception: " + e);
    }

Osseous

Answer 1

Assuming that it's the BasicDataSource is from DBCP, then yes, you are using a connection pool. However, you're recreating another connection pool on every connection acquirement. You are not really pooling connections from the same pool. You need to create the connection pool only once on application's startup and get every connection from it. You should also not hold the connection as an instance variable. You should also close the connection, statement and resultset to ensure that the resources are properly closed, also in case of exceptions. Java 7's try-with-resources statement is helpful in this, it will auto-close the resources when the try block is finished.

Here's a minor rewrite:

public final class Database {

    private static final BasicDataSource dataSource = new BasicDataSource();

    static {
        dataSource.setDriverClassName("com.mysql.jdbc.Driver");
        dataSource.setUrl("jdbc:mysql://localhost:3306/data");
        dataSource.setUsername("USERNAME");
        dataSource.setPassword("PASSWORD");
    }

    private Database() {
        //
    }

    public static Connection getConnection() throws SQLException {
        return dataSource.getConnection();
    }

}

(this can if necessary be refactored as an abstract factory to improve pluggability)

and

private static final String SQL_EXIST = "SELECT * FROM users WHERE username=? AND password=?";

public boolean exist(User user) throws SQLException {
    boolean exist = false;

    try (
        Connection connection = Database.getConnection();
        PreparedStatement statement = connection.prepareStatement(SQL_EXIST);
    ) {
        statement.setString(1, user.getUsername());
        statement.setString(2, user.getPassword());

        try (ResultSet resultSet = preparedStatement.executeQuery()) {
            exist = resultSet.next();
        }
    }       

    return exist;
}

which is to be used as follows:

try {
    if (!userDAO.exist(username, password)) {
        request.setAttribute("message", "Unknown login. Try again.");
        request.getRequestDispatcher("/WEB-INF/login.jsp").forward(request, response);
    } else {
        request.getSession().setAttribute("user", username);
        response.sendRedirect("userhome");
    }
} catch (SQLException e) {
    throw new ServletException("DB error", e);
}

In a real Java EE environement you should however delegate the creation of the DataSource to the container / application server and obtain it from JNDI. In case of Tomcat, see also for example this document: http://tomcat.apache.org/tomcat-6.0-doc/jndi-resources-howto.html

Monogram

Wow, thanks for the rewrite as well. Perfect help for someone new like me. – Osseous

will this solution be threadsafe ?Do i need to call connection.close(); – Isoniazid

@swapyonubuntu: close is automatically done with the new Java7 try-with-resources statement docs.oracle.com/javase/tutorial/essential/exceptions/… – Monogram

@Monogram why the origin example is "recreating another connection pool on every connection acquirement"? because each time it call getConnection(), it calls setDriver, setUrl.....etc? these call will recreating another connection pool? As I see your example, you put these setDriver.... to static initializer – Vicinity

@user1169587: Nope, because it calls new BasicDataSource() each time. – Monogram

Answer 2

Doesn't seem like it's pooled. You should store the DataSource in DatabaseConnection instead of creating a new one with each getConnection() call. getConnection() should return datasource.getConnection().

Creosol

Answer 3

Looks like a DBCP usage. If so, then yes. It's already pooled. And here is the default pool property value of the DBCP.

/**
* The default cap on the number of "sleeping" instances in the pool.
* @see #getMaxIdle
* @see #setMaxIdle
*/
public static final int DEFAULT_MAX_IDLE  = 8;
/**
* The default minimum number of "sleeping" instances in the pool
* before before the evictor thread (if active) spawns new objects.
* @see #getMinIdle
* @see #setMinIdle
*/
public static final int DEFAULT_MIN_IDLE = 0;
/**
* The default cap on the total number of active instances from the pool.
* @see #getMaxActive
*/
public static final int DEFAULT_MAX_ACTIVE  = 8;

Idelia

Answer 4

As a follow up to BalusC's solution, below is an implementation that I can be used within an application that requires more than one connection, or in a common library that would not know the connection properties in advance...

    import org.apache.commons.dbcp.BasicDataSource;

    import java.sql.Connection;
    import java.sql.SQLException;
    import java.util.concurrent.ConcurrentHashMap;

    public final class Database {

        private static final ConcurrentHashMap<String, BasicDataSource> dataSources = new ConcurrentHashMap();

        private Database() {
            //
        }

        public static Connection getConnection(String connectionString, String username, String password) throws SQLException {

            BasicDataSource dataSource;

            if (dataSources.containsKey(connectionString)) {
                dataSource = dataSources.get(connectionString);
            } else {
                dataSource = new BasicDataSource();
                dataSource.setDriverClassName("com.mysql.jdbc.Driver");
                dataSource.setUrl(connectionString);
                dataSource.setUsername(username);
                dataSource.setPassword(password);
                dataSources.put(connectionString, dataSource);
            }
        
            return dataSource.getConnection();
        
        }

    }

Narthex

This solutions does not work always. It is subject to race conditions despite of using ConcurrentHashMap. – Melodics

@Carlos Caldas: I was not aware of that limitation. Could you please explain how this answer could be improved? I will gladly update the answer. – Narthex

Imagine containsKey() returning false for current thread while another thread is busy creating the datasource before calling put(). Using computeIfAbsent() instead of containsKey()+put() solves this. – Monogram

With this change, ConcurrentHashMap isn't anymore necessary as long as you don't use get() on same map anywhere else. It can be a regular HashMap. – Monogram

@Monogram Thanks for that input. I'll pull up that old project, make the necessary changes, compile, and test, then update my answer accordingly. – Narthex