The right way to handle database connections in a medium-sized web application

Question

I currently support a small, small Java application (using only simple JSPs / servlets) that the intern made for internal use by the company, and I am having connection problems.

Sometimes we get errors from nowhere, such as “Application closed” or “Connection closed”, and then the whole application just stops working and the server needs to be restarted.

I don’t have much experience and I don’t have anyone to instruct me or teach me about best practices, design patterns, etc., but I’m sure this is the wrong way to do this. I read about things like DAL, DAO and DTO. Our application does not have any of them.

An entire web application (i.e. servlets) is basically populated 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 performed as follows:

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

Where Model is a class that extends HashMap and is a single row in a table.

This is the code for Database.java and wondered if anyone could point out the obvious things that are wrong (I'm sure there are a lot of them), any quick fixes that can be made, and some best practices resources with database connections / connection.

 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_FORWARD_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!

Answer 1

The class never closes the connection: this.connection.close() . Since Database is Singleton , the application does not use the connection pool (data source). For all incoming requests, only one connection is used.

Rule of thumb: get one connection in each method (possibly in a SQL statement). dataSource.getConnection() not expensive.

This is how I would reorganize the class:

• remove the public getConnection method, if it is used outside the Database class, you really have a design problem.
• remove the commit method. I believe this makes no sense, since connection.setAutoCommit(false) never called, and I don't see the rollback method
• delete the connection instance variable, get the connection to the call instead
• and close this connection correctly in the finally block for each call

Disclaimer: I don’t know how transaction processing works at the moment, so I can be wrong in # 2.

Sample code for the method of obtaining the connection:

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

I wonder how this application can work at all :-)

Answer 2

There is nothing wrong with doing it this way for simple applications. But if your application is even moderately complex, you may need to learn a simple structure such as iBatis.

A few things I would definitely do, though. First, an application can be a connection leak when an exception is thrown, to how the instructions close. All closing statements must 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(); } 

Another thing, I would make sure that you use the database connection pool for your data source. If you use this in Tomcat, we hope that there is a connection pool in the tomcat installation, and your application uses this.

EDIT: after looking at the code again, I also don’t see where the database connection actually closes. This is probably why you are running out of connections. You need to add the close method to the Database class, which calls connection.close (). And make sure you call it when you're done with your requests. Again, in the try / finally block.

Answer 3

You are using a JDBC connection is very insecure. Access to it is possible from several threads, and it is not thread safe. This is a web application and several requests may come from different users at the same time. This is a small miracle, your application does not crash more often. You can use several strategies to fix this. You can store connections in ThreadLocal or on the stack. If you intend to maintain connections on the stack, you will have to open and close them in every method call. To have a certain performance, you will have to use a connection pool. The connection pool will never hurt.