Singleton Database Class for JDBC Pool using Apache Tomcat JDBC

Question

I am trying to configure a singleton database class to allow connection pooling against Oracle XE 18c database using JDK8 and Tomcat7.

I can not compile the source because of the following error:

unreported exception SQLException; must be caught or declared to be thrown

Class source code:

package com.example.webapp.db;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

import org.apache.tomcat.jdbc.pool.DataSource;
import org.apache.tomcat.jdbc.pool.PoolProperties;
public class DatabaseManager {

    private static final DatabaseManager SINGLE_INSTANCE = new DatabaseManager();

    private DatabaseManager() throws SQLException {
        PoolProperties p = new PoolProperties();
        p.setUrl("jdbc:oracle:thin:@localhost:1521:xe");
        p.setDriverClassName("oracle.jdbc.driver.OracleDriver");
        p.setUsername("scott");
        p.setPassword("tiger");
        p.setJmxEnabled(true);
        p.setTestWhileIdle(false);
        p.setTestOnBorrow(true);
        p.setValidationQuery("SELECT 1");
        p.setTestOnReturn(false);
        p.setValidationInterval(30000);
        p.setTimeBetweenEvictionRunsMillis(30000);
        p.setMaxActive(100);
        p.setInitialSize(10);
        p.setMaxWait(10000);
        p.setRemoveAbandonedTimeout(60);
        p.setMinEvictableIdleTimeMillis(30000);
        p.setMinIdle(10);
        p.setLogAbandoned(true);
        p.setRemoveAbandoned(true);
        p.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.ConnectionState;"+
          "org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer");
        DataSource datasource = new DataSource();
        datasource.setPoolProperties(p); 

        Connection con = null;
        try {
          con = datasource.getConnection();
          Statement st = con.createStatement();
          ResultSet rs = st.executeQuery("select * from user");
          int cnt = 1;
          while (rs.next()) {
              System.out.println((cnt++)+". Host:" +rs.getString("Host")+
                " User:"+rs.getString("User")+" Password:"+rs.getString("Password"));
          }
          rs.close();
          st.close();
        } finally {
          if (con!=null) try {con.close();}catch (Exception ignore) {}
        }
    }

    public static DatabaseManager getInstance() {
        return SINGLE_INSTANCE;
    }
}

...and what is unclear about that message? It is a checked exception, so it "must be caught or declared to be thrown". As you cannot declare it in a static initializer, you need to catch it. — Hulk, Jan 07 '19 at 10:07
You made your constructor throw an `SQLException`, which needs to be caught on calling this constructor. That means each line including a `new DatabaseManager()` has to be wrapped in `try {...} catch (SQLException e)`. In your case, this will be hard due to the line being a `private static final DatabaseManager`... So maybe add a `catch` clause to the `try {...} finally {...}` you have in the constructor code and remove the `throws SQLException` from the constructor signature. — deHaar, Jan 07 '19 at 10:08
The initialization of a `DatabaseManager` should probably not fail just because it cannot currently connect to a DB. Arguably, it shouldn't even try to do something like that in a constructor. — Hulk, Jan 07 '19 at 10:22
Related: [Java unreported exception](https://stackoverflow.com/questions/2091589/java-unreported-exception) — Hulk, Jan 07 '19 at 10:29
Also see https://stackoverflow.com/questions/908672/why-do-i-get-exception-must-be-caught-or-declared-to-be-thrown-when-i-try-to — Hulk, Jan 07 '19 at 10:32
Another aspect: https://stackoverflow.com/questions/2284502/singleton-and-exception (but that is for cases where the exception really should prevent your entire application from working) — Hulk, Jan 07 '19 at 10:36

Ramesh Subramanian · Answer 1 · 2019-01-07T10:25:11.290

2

Change the code as shown below, You have to catch the exception.

private static  DatabaseManager SINGLE_INSTANCE = null;
static {
    try {
        SINGLE_INSTANCE = new DatabaseManager();
    }
    catch(Exception e) {
        e.printStackTrace();
    }
}

edited Jan 07 '19 at 10:25

answered Jan 07 '19 at 10:11

Ramesh Subramanian

875
1
8
25

While this will make it compile, it may not be a good idea because you now need null-checks whenever you want to access `SINGLE_INSTANCE`. – Hulk Jan 07 '19 at 10:14
Yes. As Hulk told , null check is required & modified the method `public static DatabaseManager getInstance() throws SQLException { if(SINGLE_INSTANCE == null) { synchronized(DatabaseManager.class){ if(SINGLE_INSTANCE == null) { SINGLE_INSTANCE = new DatabaseManager(); } } } return SINGLE_INSTANCE; }}` – Ramesh Subramanian Jan 07 '19 at 10:22
It would be better to throw `ExceptionInInitializerError` instead of ignoring the error and acting as if nothing happened. – Mark Rotteveel Jan 08 '19 at 12:40

score 2 · Accepted Answer · answered Jan 07 '19 at 10:36

I think you should either throw or handle the SQLException in the getInstance() and use a try-with resource (automatically closes resources) instead of a finally block. You should not make your instance a constant using a constructor that throws an Exception for initialization.

package com.example.webapp.db;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

import org.apache.tomcat.jdbc.pool.DataSource;
import org.apache.tomcat.jdbc.pool.PoolProperties;

public class DatabaseManager {

    // not final anymore and null as default
    private static DatabaseManager instance = null;

    private DatabaseManager() {
        PoolProperties p = new PoolProperties();
        p.setUrl("jdbc:oracle:thin:@localhost:1521:xe");
        p.setDriverClassName("oracle.jdbc.driver.OracleDriver");
        p.setUsername("scott");
        p.setPassword("tiger");
        p.setJmxEnabled(true);
        p.setTestWhileIdle(false);
        p.setTestOnBorrow(true);
        p.setValidationQuery("SELECT 1");
        p.setTestOnReturn(false);
        p.setValidationInterval(30000);
        p.setTimeBetweenEvictionRunsMillis(30000);
        p.setMaxActive(100);
        p.setInitialSize(10);
        p.setMaxWait(10000);
        p.setRemoveAbandonedTimeout(60);
        p.setMinEvictableIdleTimeMillis(30000);
        p.setMinIdle(10);
        p.setLogAbandoned(true);
        p.setRemoveAbandoned(true);
        p.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.ConnectionState;"
                + "org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer");
        javax.sql.DataSource datasource = new DataSource();
        datasource.setPoolProperties(p);

        // use a try-with resource to get rid of the finally block...
        try (Connection con = datasource.getConnection()) {
            Statement st = con.createStatement();
            ResultSet rs = st.executeQuery("select * from user");
            int cnt = 1;

            while (rs.next()) {
                System.out.println((cnt++) + ". Host:" + rs.getString("Host")
                        + " User:" + rs.getString("User")
                        + " Password:" + rs.getString("Password"));
            }

            rs.close();
            st.close();
        // ... and handle the exception
        } catch (SQLException e) {
            System.err.println("SQLException while constructing the instance of DatabaseManager");
            e.printStackTrace();
        }
    }

    public static DatabaseManager getInstance() {
        // check for null here:
        if (instance == null) {
            instance = new DatabaseManager();
        }
        return instance;
    }
}

Maybe, creating an initialization method for the database connection would be a better idea than initializing everything in the constructor, but that is kind of opinion based.

@Hulk OK, then what to do? Synchronize or make it `volatile`? What is your suggestion? — deHaar, Jan 07 '19 at 10:42
I'm not sure - I would probably move the DB - accessing code out of the constructor. Or perhaps it shouldn't even really be a singleton. Singleton means it **must** never be constructed more than once, and if this was really the requirement it would be ok to crash/terminate the application in case of failure (but I doubt that is the desired behavior here). — Hulk, Jan 07 '19 at 10:50
@Hulk Yes, I know [the discussion about Singletons](https://stackoverflow.com/questions/137975/what-is-so-bad-about-singletons). Sometimes people just have to use them due to an order or a special task concerning Singletons. We don't know if this is the case here, so I just tried to simpilfy the code that was provided. Well, if it is not thread safe, it is not a good answer. — deHaar, Jan 07 '19 at 10:57
You could just make the `getInstance`-method `synchronized`- this will hurt performance, but keep it simple and relatively correct. However, that will retry the DB connection on each call until it succeeds and may still return `null` (Because you swallowed the exception). That would be unexpected for a `getInstance()`, but throwing an exception would also be a surprise, so I don't really see a good way out here without significant redesign. Not quite serious: `public Optional tryToGetInstance()`... — Hulk, Jan 07 '19 at 11:12

Singleton Database Class for JDBC Pool using Apache Tomcat JDBC

2 Answers2