Connection Pool implementation in java

Question

I am implementing a conenction pool (JDBC connections and SMPP connections).I know there are several well tested connection pools out there.But I just want to implement on my own. I am using it in multithreading environment.This is more of my personal interest.My implementation goes like this. I create a ConcurrentLinkedQueue and push the connections to the queue. Each time a thread requests a connection, connection is popped out from queue. Once job is done threads push connection back to queue. My connection poll implementation class is as shown below.

import java.util.concurrent.ConcurrentLinkedQueue;

import org.apache.log4j.Logger;
import org.jsmpp.bean.BindType;
import org.jsmpp.bean.NumberingPlanIndicator;
import org.jsmpp.bean.TypeOfNumber;
import org.jsmpp.session.BindParameter;
import org.jsmpp.session.SMPPSession;


public class SMPPConnectionPool {
    static ConcurrentLinkedQueue<SMPPSession> connectionPool = null;
    static Logger LOG = null;

    static 
    {
        LOG = LogManager.getLogger(SMPPConnectionPool.class);
        connectionPool= new ConcurrentLinkedQueue<SMPPSession>();
    }

    /* This method returns session from queue .If no sessions exist in the queue,then a new session will be created and returned.
     * This method use QueryGparams APi to read conenction related data from gparams
     * */
    public static SMPPSession getConenction()
    {
        SMPPSession session=connectionPool.poll();
        if(session!=null)
        {
            System.out.println("Thread "+Thread.currentThread().getName() +" got "+session.getSessionId());
            LOG.info("Thread "+Thread.currentThread().getName() +" got "+session.getSessionId());
            return session;
        }
        else
        {
            SMPPSession smppSession = new SMPPSession();
            try {
                String host = QueryGparams.getGparamAsString(NotificationConstants.SMSC_HOST);
                int port = QueryGparams.getGparamAsInteger(NotificationConstants.SMSC_PORT);
                String systemId = QueryGparams.getGparamAsString(NotificationConstants.SMSC_SYSTEM_ID);
                String password = QueryGparams.getGparamAsString(NotificationConstants.SMSC_PASSWORD);
                if(host == null || systemId == null || password == null || port == 0)
                {
                    String errorMessage = "Following parameters are null \n";
                    if(host == null) {
                        errorMessage = errorMessage + "host is null";
                    }
                    if(systemId == null) {
                        errorMessage = errorMessage + "systemId is null";
                    }
                    if(password == null) {
                        errorMessage = errorMessage + "password is null";
                    }
                    if(port == 0) { //TODO Need to change this if QueryGParams API will not return zero when port number is not specified 
                        errorMessage = errorMessage + "port is null";
                    }
                    throw new Exception(errorMessage);
                }
                smppSession
                .connectAndBind(host,port, new BindParameter(
                        BindType.BIND_TRX, systemId,
                        password, "",
                        TypeOfNumber.UNKNOWN,
                        NumberingPlanIndicator.UNKNOWN, null));
                LOG.info("Session has been created.Session id is "+smppSession.getSessionId());
            } catch (Exception e) {
                LOG.error(CommonUtilities.getExceptionString(e));
            }
            System.out.println("Thread "+Thread.currentThread().getName() +" got "+smppSession.getSessionId());
            LOG.info("Thread "+Thread.currentThread().getName() +" got "+smppSession.getSessionId());
            return smppSession;
        }
    }



    //This method pushes conenction back to queue ,to make it available for other threads. 
    public static void pushConenction(SMPPSession smppSession)
    {
        boolean isInserted=connectionPool.offer(smppSession);
        if(isInserted)
        {
            LOG.info("Pushed the conenction back to queue");
            System.out.println("Pushed the conenction back to queue");
        }
        else
        {
            LOG.info("Failed to push the session back to queue");
            System.out.println("Failed to push the session back to queue");
        }
    }
    public static void closeSessions()
    {
        while(connectionPool!=null && connectionPool.size()>0)
        {
            SMPPSession session=connectionPool.poll();
            session.unbindAndClose();
            LOG.info("Closed the session");
        }
    }

}

I just want to know the problems with this implementation.Please advice.I want do the same for JDBC connection pool implementation

Answer 1

1) boolean isInserted=connectionPool.offer(smppSession); and subsequent analisys does not make sense since ConcurrentLinkedQueue is unbounded and its offer() always returns true. See API

2) I would use a BlockingQueue and make threads to wait for an active thread to return a connection to pool if max active threds is reached

3) I would initialize static fields this way

static ConcurrentLinkedQueue<SMPPSession> connectionPool = new ConcurrentLinkedQueue<SMPPSession>();
static Logger LOG = LogManager.getLogger(SMPPConnectionPool.class);

4) To avoid unnecessary string concatination you could use this idiom

if (LOG.isInfoEnabled()) {
   LOG.info("Thread "+Thread.currentThread().getName() +" got "+smppSession.getSessionId());
}

5) this seems suspicious

} catch (Exception e) {
    LOG.error(CommonUtilities.getExceptionString(e));
}
System.out.println("Thread "+Thread.currentThread().getName() +" got "+smppSession.getSessionId());

you catch exception and continue as if all is OK

Answer 2

Some of the problems I see with the implementation are the following:

1. There is no timeout configured for connectAndBind. In case of network partitioning or packet losses, there is a high probability that the getConnection call might just be stuck.

2. In case of network outages, its possible that the TCP connection between the data source and the client breaks down. Thus, the connection which is held by the pool will be stale. There seems to be no mechanism present which would "refresh" the connections. Many connection pool frameworks provide testConnections to be performed either in background or just before checking out the connections (which of course adds latency to the overall getConnection call) to cope up with this.