How to properly close datasource connection?

Question

I have this class but Im not sure on how to properly close connection because Im still having this error even though I only have 3 users logged in but with multiple sql queries.

> com.mysql.jdbc.exceptions.jdbc4.MySQLNonTransientConnectionException:
> Data source rejected establishment of connection,  message from
> server: "Too many connections"

import java.io.File;
import java.io.IOException;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;

import javax.sql.DataSource;

public class UserDaoImpl implements UserDao
{

    DataSource dataSource;

    public DataSource getDataSource()
    {
            return this.dataSource;
    }

    public void setDataSource(DataSource dataSource)
    {
            this.dataSource = dataSource;
    }


    public boolean isValidUser(String username, String password) throws SQLException
    {       
        PreparedStatement pstmt = null;
        ResultSet resultSet = null;
        boolean rt = false;
        try{
            PasswordEncryptor pws = new PasswordEncryptor();
            String encryptedPass = pws.encrypt(password);

            String query = "Select count(1) from userdetails where username = ? and password = ?";
            pstmt = dataSource.getConnection().prepareStatement(query);
            pstmt.setString(1, username);
            pstmt.setString(2, encryptedPass);
            resultSet = pstmt.executeQuery();
            if (resultSet.next()){
                    rt =  (resultSet.getInt(1) > 0);
            }
            else{
                rt = false;
            }
    }
    catch(Exception e){
        e.printStackTrace();

    }
    finally{
        resultSet.close();
        pstmt.close();
        dataSource.getConnection().close();
    }

        return rt;  
    }
}

SpringConfiguration.xml

    <bean name="userDao" class="com.spring.acadconnect.services.UserDaoImpl">
   <property name="dataSource" ref="dataSource"></property>
   </bean>

<bean id="dataSource" class="org.springframework.jdbc.datasource.DriverManagerDataSource">

    <property name="driverClassName" value="com.mysql.jdbc.Driver" />

    <property name="url" value="jdbc:mysql://localhost:3306/acadconnect" />

    <property name="username" value="root" />

    <property name="password" value="" />

</bean>

Answer 1

Notice that you're calling .getConnection() multiple times. Although the documentation could be clearer on this front DataSource.getConnection() actually opens a new connection (as opposed to returning an existing one) thus you need to close each instance returned from that method.

As .getConnection() creates a new instance every time it's called this line is a connection leak, since it's not closing the connection being returned:

pstmt = dataSource.getConnection().prepareStatement(query);

And this line wastefully opens a new connection only to immediately close it:

dataSource.getConnection().close();

---

It looks like you're trying to open and close a separate connection for each invocation of isValidUser() (since you're closing the connection at the end of that method call). Even if you fix the leak described above, that's not how connections are intended to be used. Instead you should share one connection (or a small number of them) across your application. So when your program starts up you open such a connection, and once the whole program no longer needs the connection (often shortly before terminating) you close it.

This sort of behavior is commonly implemented by dependency injection, where you construct your connections and other resources and then pass them into whatever objects need them - this decouples resource management from the code that uses those resources. As a simplistic example:

public static void main(String[] args) {
  DataSource dataSource = createDataSource();
  try (Connection connection = dataSource.getConnection()) {
    runProgram(connection);
  }
}


/**
 * this method doesn't need to worry about closing the Connection,
 * it trusts that its caller will be responsible for that.
 */
private static void runProgram(Connection connection) {
  // ...
}

As a rule of thumb, objects should only be responsible for closing objects they construct, and should avoid closing objects they are passed. In your current code UserDaoImpl is opening the connection, so it should be responsible for closing it, but I'm suggesting passing in the Connection instead.

Answer 2

dataSource.getConnection() will always return you a new connection, therefore you are not closing the connection that you think. You must use DataSourceUtils.getConnection() to get the active connection of the current thread or else store the reference returned like conn = dataSource.getConnection() and the call conn.close()