11

I am refactoring others code. The one thing I notice is that of the manner on how the system is getting a connection from the connection pool.

Sample is like this. On every call of the service method, the system is making a context lookup on the JNDI for the datasource.

public class CheckinServlet extends HttpServlet {

    public void doPost(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException {

        try {
            //Obtain Connection
            InitialContext initialContext = new InitialContext();
            javax.sql.DataSource ds = (javax.sql.DataSource) initialContext
                    .lookup("jdbc/mysqldb");
            java.sql.Connection conn = ds.getConnection();
            //business logic
            //redirect
        } finally {
            conn.close();
        }
    }
}

I do think that there is a performance hit on doing this every time. I am thinking of another way around these on how to retrieve a connection from a connection pool.

I am thinking about using the servlet's init() method but I think that is not optimal.

halfer
  • 19,824
  • 17
  • 99
  • 186
Mark Estrada
  • 9,013
  • 37
  • 119
  • 186

4 Answers4

15

Do it once in a ServletContextListener instead of everytime in init() of many servlets. The contextInitialized() method is executed only once during webapp's startup.

public class Config implements ServletContextListener {
    private static final String ATTRIBUTE_NAME = "config";
    private DataSource dataSource;

    @Override
    public void contextInitialized(ServletContextEvent event) {
        ServletContext servletContext = event.getServletContext();
        String databaseName = servletContext.getInitParameter("database.name");
        try {
            dataSource = (DataSource) new InitialContext().lookup(databaseName);
        } catch (NamingException e) {
            throw new RuntimeException("Config failed: datasource not found", e);
        }
        servletContext.setAttribute(ATTRIBUTE_NAME, this);
    }

    @Override
    public void contextDestroyed(ServletContextEvent event) {
        // NOOP.
    }

    public DataSource getDataSource() {
        return dataSource;
    }

    public static Config getInstance(ServletContext servletContext) {
        return (Config) servletContext.getAttribute(ATTRIBUTE_NAME);
    }
}

Configure it as follows in web.xml:

<context-param>
    <param-name>database.name</param-name>
    <param-value>jdbc/mysqldb</param-value>
</context-param>
<listener>
    <listener-class>com.example.Config</listener-class>
</listener>

You can obtain it in your servlet as follows (init() or doXXX() method, you choose):

DataSource dataSource = Config.getInstance(getServletContext()).getDataSource();

I'd however refactor it a step further, JDBC code should preferably be placed in its own classes, not in servlets. Lookup the DAO pattern.

BalusC
  • 1,082,665
  • 372
  • 3,610
  • 3,555
  • 1
    Hi sir, Thank you for the always detailed answer. But one thing keeps me thinking about this. Why is it that you place the Datasource object as a context wide parameters? Cant we just make the getDataSource() method a static one? I really like this answer but I wanted to further learn the reason of doing this. Thank you.. – Mark Estrada Dec 21 '10 at 02:42
  • See it als good design. A `DataSource` is specific to a single `Config` instance, not to multiple `Config` instances. Even though there's in this particular case only one. – BalusC Dec 21 '10 at 02:51
  • Does the database connection get closed when contextDestroyed(ServletContextEvent) is called? –  Apr 07 '11 at 15:22
  • 3
    @robert: You should definitely not keep the connection open for that long. You need to close it youself according the normal JDBC idiom: in the `finally` block of the `try` block where it is been opened. Please note that `DataSource != Connection`. See also this answer: http://stackoverflow.com/questions/2313197/jdbc-mysql-connection-pooling-practices/2313262#2313262 In the future, please press `Ask Question` if you have questions, we deserve points :) – BalusC Apr 07 '11 at 15:24
  • I was thinking about pressing Ask Question, but didn't. I will next time! –  Apr 07 '11 at 15:29
3

I just did some testing with this, and found that jndi lookup time is not that heavy. 50.000 lookups in about 1 sec here.

So in many cases, I don't see a reason for caching the DataSource at all.

Problem with caching is that you might end up with a stale DataSource, forcing you to restart your app if you change anything related to the datasource definition.

Martin Wickman
  • 19,662
  • 12
  • 82
  • 106
3

The method I have used in the past is to create a singleton class that holds the datasource

E.g.

public class DatabaseConnectionManager {

    DataSource ds;

    public void init() {
        InitialContext initialContext = new InitialContext();
        ds = (javax.sql.DataSource)initialContext.lookup("jdbc/mysqldb");
    }

    public Connection getConnection() {
        if(ds == null) init();

        return ds.getConnection();
    }
}

This means that you have a shared reference to your datasource, taking away the jndi lookup overhead.

Codemwnci
  • 54,176
  • 10
  • 96
  • 129
2

Adding to what's said, there's a design pattern called Service Locator, which is a basically a singleton containing a registry called "service" that holds your JNDI objects.

Basically, if the object isn't found in the registry, the service is taken from the JNDI pool and registered in the registry. The next call will simply pull the object from registry.

Hope this helps.

Buhake Sindi
  • 87,898
  • 29
  • 167
  • 228