CODEWITHSUNDEEP
javamultithreadingjdbcconnection-pooling
arya
arya

Reputation: 170

JDBC Connection pooling issue - Deadlock while getting connection

I am using JDBC connection pooling in Tomcat. To retrieve connections I have defined a connection factory as below:

public class ConnectionManager {

   // reference to the ConnectionManager
   private static ConnectionManager instance = null;
   // Connection to MySQL database
   private Connection connect = null;

   private static DataSource ds = null;
   // Logger
   public static final Logger logger = Logger
         .getLogger(ConnectionManager.class);

   static {

      try {
         Context initCtx = new InitialContext();
         Context envCtx = (Context) initCtx.lookup("java:comp/env");
         ds = (DataSource) envCtx.lookup("jdbc/ConnectionManager");
      } catch (NamingException e) {
         // TODO Auto-generated catch block
         e.printStackTrace();
      }
   }

   /**
    * Private Constructor .. since its a singleton
    * 
    */
   private ConnectionManager() {

   }

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

   public Connection getDbConnection() {
      Connection conn = null;

      try {
         synchronized (DataSource.class) {

            conn = ds.getConnection();
         }
      } catch (SQLException e) {
         // TODO Auto-generated catch block
         e.printStackTrace();
      }

      return conn;
   }

   public void closeDbConnection() throws SQLException {
      conn.close();
   }
}

Now I see that my code always gets stuck at conn = ds.getConnection(); line. Please let me know what I am doing wrong. From my DAO methods I am using the following to get connection: conn = ds.getConnection();

Clearly its a multi-threading issue. What should I do?

Upvotes: 3

Views: 9240

Answers (4)

Perception
Perception

Reputation: 80603

Most of your class seems geared around retrieving the JNDI datasource and using it to create connections. Not necessarily a bad idea but in this case you have introduced some bugs into your program with the additional complexity.

First off, your singleton is not a singleton. Your are not synchronizing the getInstance method so its possible to multiple threads to invoke this method at the same time. The best method in Java (unfortunately) for implementing singletons is via an enum:

public enum ConnectionManager {
    INSTANCE;
}

Your second significant issue is that you are synchronizing on a class that you don't explicitly control. There is nothing preventing third party JARs or even other classes in your own application from synchronizing on the DataSource class, making it a rife target for deadlocking issues. I would take out all the superfluous methods from the class and remove the synchronize block:

public enum ConnectionManager {
    INSTANCE;

    private DataSource ds = null;

    ConnectionManager() {
      try {
         final Context initCtx = new InitialContext();
         final Context envCtx = (Context) initCtx.lookup("java:comp/env");
         ds = (DataSource) envCtx.lookup("jdbc/ConnectionManager");
      } catch (NamingException e) {
         e.printStackTrace();
      }
    }

   public Connection getConnection() throws SQLException {
      if(ds == null) return null;

      return ds.getConnection();
   }
}

Now, most datasource implementations are thread safe in my experience, so the above code should work most of the time. But, we shouldn't rely on implementations we cannot control, so lets add a safe synchronization to the code, like so:

public enum ConnectionManager {
    INSTANCE;

    private DataSource ds = null;
    private Lock connectionLock = new ReentrantLock();

    ConnectionManager() {
      try {
         final Context initCtx = new InitialContext();
         final Context envCtx = (Context) initCtx.lookup("java:comp/env");
         ds = (DataSource) envCtx.lookup("jdbc/ConnectionManager");
      } catch (NamingException e) {
         e.printStackTrace();
      }
    }

   public Connection getConnection() throws SQLException {
      if(ds == null) return null;

      Connection conn = null;
      connectionLock.lock();
      try {
          conn = ds.getConnection();
      } finally {
          connectionLock.unlock();
      }

      return conn;
   }
}

You don't have to add wrapper methods to close the connection, that is the responsibility of the calling code. Good luck.

Upvotes: 4

Andrey Nudko
Andrey Nudko

Reputation: 707

That code is virtually guaranteed to cause connection leaks in a multithreaded system. closeDbConnection() closes only the last connection borrowed from pool - so if 10 threads have called getDbConnection(), and after that closeDbConnection(), only 1 connection is closed and 9 still alive. Repeat that several times and pool is exhausted (unless connection is cleaned up in finalize(), but that's probably not the case). I would get rid of the whole class, or reworked it to act only as a datasource locator.

Upvotes: 0

Asif
Asif

Reputation: 5038

Well I would say first try out your dataSource is working or not with a test source.
I suggest look at Apache Tomcat JNDI Data Resource How To, for Apache Tomcat 6.0 and for Apache Tomcat 7.0.

Look at the instructions carefully and analyse what's going wrong in your code, then update your question with specific problem.

Upvotes: 0

Akash Yadav
Akash Yadav

Reputation: 2421

@arya, seems like you are having the problem of connection leak, and because of that the pool is getting exhausted and the code just waits till it gets a new connection, To analyze the problem , use any of the database monitoring tools, or manually try to trace the leak (The point in code where you have consumed a connnection but forgot to return it to the pool after use).

Upvotes: 0

Related Questions