Proper way of handling database connections in a medium sized web application

I'm currently maintaining a small-medium sized java web application (using only plain JSPs/Servlets) that an intern made for internal company use and I'm having some trouble with connections.

Sometimes just out of nowhere, we get errors like "Statement is closed" or "Connection is closed" and then the whole application would just stop working and the server has to be restarted.

I don't have a lot of experience and I don't have anyone to mentor or teach me regarding best practices, design patterns, etc. but I'm pretty sure this is not the right way to do this. I've read about stuff like DALs, DAOs, and DTOs. Our app has none of those.

The whole web application (ie. the servlets) are basically filled with calls similar to the following:

Database db = Database.getInstance();
db.execute("INSERT INTO SomeTable VALUES (a, b, c)");
db.execute("UPDATE SomeTable SET Col = Val");

SELECTs are done like so:

ArrayList<Model> results = Model.fetch("SELECT * FROM SomeTable");

Where Model is a class that extends HashMap and represents a single Row in a Table.

This is the code for Database.java and was wondering if someone can point out obvious things that are wrong (I'm pretty sure there are a lot), any quick fixes that can be done and some resources on best practices with regards to database connections / connection handling.

package classes;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.HashMap;

import javax.naming.InitialContext;
import javax.naming.NamingException;
import javax.sql.DataSource;

public final class Database {

    public static Database getInstance() {
        if (Database.instance == null) {
            Database.instance = new Database();
        }
        return Database.instance;
    }

    // Returns the results for an SQL SELECT query.
    public ArrayList<HashMap<String, Object>> fetch(String sql) {

        ArrayList<HashMap<String, Object>> results = new ArrayList<HashMap<String, Object>>();

        try {

            PreparedStatement stmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWAR开发者_如何学运维D_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            ResultSet rs = stmt.executeQuery();
            this.doFetch(rs, results);
            stmt.close();

        } catch (SQLException e) {
            this.handleException(e, sql);
        }

        return results;
    }

    public ArrayList<HashMap<String, Object>> fetch(String sql, ArrayList<Object> parameters) {

        ArrayList<HashMap<String, Object>> results = new ArrayList<HashMap<String, Object>>();

        try {

            // Bind parameters to statement.
            PreparedStatement pstmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            for (int i=0; i<parameters.size(); i++) {
                pstmt.setObject(i+1, parameters.get(i));
            }

            ResultSet rs = pstmt.executeQuery();
            this.doFetch(rs, results);
            pstmt.close();

        } catch (SQLException e) {
            this.handleException(e, sql, parameters);
        }

        return results;
    }

    public int execute(String sql) {
        int result = 0;
        try {
            Statement stmt = this.connection.createStatement();
            result = stmt.executeUpdate(sql);
            stmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql);
        }
        return result;
    }

    public int execute(String sql, ArrayList<Object> parameters) {
        int result = 0;
        try {
            PreparedStatement pstmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            for (int i=0; i<parameters.size(); i++) {
                if (parameters.get(i) == null) {
                    pstmt.setNull(i+1, java.sql.Types.INTEGER);
                } else {
                    pstmt.setObject(i+1, parameters.get(i));
                }
            }
            result = pstmt.executeUpdate();
            pstmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql, parameters);
        }
        return result;
    }

    public void commit() {
        try {
            this.connection.commit();
        } catch (SQLException e) {
            System.out.println("Failed to commit transaction.");
        }
    }

    public Connection getConnection() {
        return this.connection;
    }


    private static Database instance;
    private static DataSource dataSource = null;
    private Connection connection;

    private Database() {
        this.connect();
        this.execute("SET SCHEMA " + Constant.DBSCHEMA);
    }

    private void connect() {
        Connection connection = null;
        if (dataSource == null) {
            try {
                InitialContext initialContext = new InitialContext();
                dataSource = (DataSource)initialContext.lookup(
                        Constant.DEPLOYED ? Constant.PROD_JNDINAME : Constant.TEST_JNDINAME);
            } catch (NamingException e) {
                e.printStackTrace();
            }
        }
        try {
            connection = dataSource.getConnection();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        this.connection = connection;
    }

    // Fetches the results from the ResultSet into the given ArrayList.

    private void doFetch(ResultSet rs, ArrayList<HashMap<String, Object>> results) throws SQLException {
        ResultSetMetaData rsmd = rs.getMetaData();

        ArrayList<String> cols = new ArrayList<String>();           
        int numCols = rsmd.getColumnCount();

        for (int i=1; i<=numCols; i++) {
            cols.add(rsmd.getColumnName(i));
        }

        while (rs.next()) {
            HashMap<String, Object> result = new HashMap<String, Object>();
            for (int i=1; i<=numCols; i++) {
                result.put(cols.get(i-1), rs.getObject(i));
            }
            results.add(result);
        }

        rs.close();
    }

    private void handleException(SQLException e, String sql) {
        System.out.println("SQLException " + e.getErrorCode() + ": " + e.getMessage());
        System.out.println("Statement: " + sql);
        ExceptionAdapter ea = new ExceptionAdapter(e);
        ea.setSQLInfo(e, sql);
        throw ea;
    }

    private void handleException(SQLException e, String sql, ArrayList<Object> parameters) {
        if (parameters.size() < 100) {
            System.out.println("SQLException " + e.getErrorCode() + ": " + e.getMessage());
            System.out.println("PreparedStatement: " + sql.replace("?", "[?]"));
            System.out.println("Parameters: " + parameters.toString());
        }
        ExceptionAdapter ea = new ExceptionAdapter(e);
        ea.setSQLInfo(e, sql, parameters);
        throw ea;
    }
}

Thanks!

---

The class does never, ever close a connection: this.connection.close(). As Database is a Singleton the application does not make use of the connection pool (the datasource). Only one connection is used for all incoming requests.

Rule of thumb: get one connection per method (maybe per SQL statement). dataSource.getConnection() is not expensive.

This is how I would refactor the class:

1. remove the public getConnection method, if it used outside the Databaseclass you really have a design problem
2. remove the commit method. I suppose it does not make sense as connection.setAutoCommit(false) is never called and I don't see a rollback method
3. remove the instance variable connection, instead obtain a connection per call
4. and properly close this connection in the finally block of each call

Disclaimer: No idea how your transaction handling works at the moment, so I may be wrong with #2.

Sample code for a method to obtain a connection:

Connection c = null;
try {
    c = this.dataSource.getConnection();
    c.executeStatement("select * from dual");
} catch (SQLException e) {
    // handle...
} finally {
    closeConnection(c);
}

Interesting how this app can work at all :-)

---

There is nothing wrong with doing it this way for simple apps per-say. But if your application is even moderately complex, you may want to look into a simple framework such as iBatis.

A couple of things I would definitely do though. For one, the application could be leaking connections when an exception is thrown do to the way the statements are being closed. All the close statements should be moved inside finally blocks.

So instead of:

try {
            Statement stmt = this.connection.createStatement();
            result = stmt.executeUpdate(sql);
            stmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql);
        }

Do this instead:

Statement stmt = null;
try {
        stmt = this.connection.createStatement();
        result = stmt.executeUpdate(sql);
    } catch (SQLException e) {
        this.handleException(e, sql);
    } finally {
        if (stmt != null) stmt.close();
    }

The other thing is I would make sure you are using a database connection pool for your datasource. If you are running this in Tomcat, hopefully there is a connection pool defined in the tomcat installation, and your application is using that.

EDIT: After looking at the code again, I'm also not seeing where the database connection is actually being closed. This is probably why you run out of connections. YOu need to add a close method to the Database class, that calls connection.close(). And make sure you are calling it when you are finished with your queries. Again, in a try/finally block.

---

You are using JDBC connection in a very unsafe way. It can be accessed from multiple threads and it is not thread safe. This is a web application and multiple requests can come from different users at the same time. It's a small miracle your application is not crashing more frequently. You can use several strategies to fix that. You can store connections in a ThreadLocal or on the stack. If you are going to keep connections on the stack, you will have to open and close them within each method call. For this to have some performance, you will have to use the connection pool. Connection pool wouldn't hurt in any case.

---

To answer your question about design principles, this object is essentially a DAO object, it just doesn't use the naming convention, also a large application would have several of these objects for different types of data (maybe using a base DAO object that they all inherit from).

The broad idea is that A DAO is a central place where database connections are handled so that you don't have all that code in a Controller object.

Aside from the shortcomings already pointed out by others, this is some solid code written by someone who understands object oriented programming very well. My recommendation is to change the object from a singleton and to manage the database connection with a connection pool(as others have already mentioned).

This seems to be a highly abstracted object that returns an arraylist of maps(key value pairs), that can be used for different datatypes which is then used in the Model objects or the datatypes to build java objects with the information returned.