57
votes

I'm running a web application on Tomcat. I have a class that handles all DB queries. This class contains the Connection object and methods that returns query results.

This is the connection object:

private static Connection conn = null;

It has only one instance (singleton).

In addition, I have methods that execute queries, such as search for a user in the db:

public static ResultSet searchUser(String user, String pass) throws SQLException

This method uses the static Connection object. My question is, is my use in static Connection object thread safe? Or can it cause problems when a lot of users will call the searchUser method?

2

2 Answers

90
votes

is my use in static Connection object thread safe?

Absolutely not!

This way the connection going to be shared among all requests sent by all users and thus all queries will interfere with each other. But threadsafety is not your only problem, resource leaking is also your other problem. You're keeping a single connection open during the entire application's lifetime. The average database will reclaim the connection whenever it's been open for too long which is usually between 30 minutes and 8 hours, depending on DB's configuration. So if your web application runs longer than that, the connection is lost and you won't be able to execute queries anymore.

This problem also applies when those resources are held as a non-static instance variable of a class instance which is reused multiple times.

You should always acquire and close the connection, statement and resultset in the shortest possible scope, preferably inside the very same try-with-resources block as where you're executing the query according the following JDBC idiom:

public User find(String username, String password) throws SQLException {
    User user = null;

    try (
        Connection connection = dataSource.getConnection();
        PreparedStatement statement = connection.prepareStatement("SELECT id, username, email FROM user WHERE username=? AND password=md5(?)");
    ) {
        statement.setString(1, username);
        statement.setString(2, password);

        try (ResultSet resultSet = statement.executeQuery()) {
            if (resultSet.next()) {
                user = new User();
                user.setId(resultSet.getLong("id"));
                user.setUsername(resultSet.getString("username"));
                user.setEmail(resultSet.getString("email"));
            }
        }
    }       

    return user;
}

Note that you should not return a ResultSet here. You should immediately read it and map it to a non-JDBC class and then return it, so that the ResultSet can safely be closed.

If you're not on Java 7 yet, then use a try-finally block wherein you manually close the closeable resources in the reverse order as you've acquired them. You can find an example here: How often should Connection, Statement and ResultSet be closed in JDBC?

If you worry about connecting performance, then you should be using connection pooling instead. This is built-in into many Java EE application servers and even barebones servletcontainers like Tomcat supports it. Just create a JNDI datasource in the server itself and let your webapp grab it as DataSource. It's transparently already a connection pool. You can find an example in the first link of the list below.

See also:

1
votes

If you are only running Select queries (searchUser sounds like only selecting data) there will be no issues, apart from thread contention.

As far as I know, a Connection can only handle one query at a time, so by using a single instance you will essentially serialize database access. But this does not necessarily mean, it is always safe to access a database like this in a multi threaded environment. There might still be issues if concurrent accesses are interleaving.