Connection Pool in Java 8 using Strategy and Factory Pattern How can I improve the following code and features? 
It is based on active911's C++ connection pool. 

The full code and unit tests are here : https://github.com/spakai/connection_pool

the README which explains the code is here : https://github.com/spakai/connection_pool/blob/master/README.md

This project uses Strategy and Factory Design Pattern. This allows ConnectionPool class to be generic. The type of database supported is determined by the type of Factory object that is injected during runtime. The specific factory creates required specific connection class that contains the actual connection to the database. So for example , if you need a MySQL connection pool, pass in a MySQLJDBConnectionFactory object.

package com.spakai;

import java.time.Duration;
import java.time.Instant;
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;

public class ConnectionPool {

  private final ConcurrentLinkedQueue&lt;JdbConnection&gt; pool = new ConcurrentLinkedQueue&lt;&gt;();

  private final Map&lt;JdbConnection,Instant&gt; borrowed = new ConcurrentHashMap&lt;&gt;();

  private final JdbConnectionFactory factory;

  private final long leaseTimeInMillis;

  /**
   * Creates connection objects and pushes it into queue.
   * @param factory Used to create JDBConnection implementations.
   * @param poolSize Number of JDBConnection implementations to create.
   * @param leaseTimeInMillis How long the client can use the connection before it expires.
   */

  public ConnectionPool(JdbConnectionFactory factory, int poolSize, long leaseTimeInMillis) {
    for (int i = 0; i &lt; poolSize; i++) {
      pool.add(factory.create());
    }

    this.factory = factory;
    this.leaseTimeInMillis = leaseTimeInMillis;
  }

  /**
   * Get a JdbConnection object either by the ones available in the queue or replace
   * the first expired connection. When a connection is given to a client, it is tagged with
   * the current time. This enables us to check the duration it has been out and replace if
   * required.
   * @return JDBConnection This contains the actual jdbc connection object to db.
   * @throws ConnectionPoolException Throws if no available connections
   */

  public JdbConnection borrow() throws ConnectionPoolException {
    if (pool.size() &gt; 0) {
      borrowed.put(pool.peek(),Instant.now());
      return pool.remove();
    } else {
      return createReplacementIfExpiredConnFound();
    }
  }

  /**
   * Return a JdbConnection object back to the pool.
   *
   * @param jdbConnection The object retrieved from the pool via borrow()
   * @throws ConnectionPoolException Throws if connection has already been 
   *        returned or forced to expire
   */

  public void forfeit(JdbConnection jdbConnection) throws ConnectionPoolException {
    if (borrowed.containsKey(jdbConnection)) {
      borrowed.remove(jdbConnection);
      pool.add(jdbConnection);
    } else {
      throw new ConnectionPoolException("Connection already returned or forced to expire");
    }
  }

  private JdbConnection createReplacementIfExpiredConnFound() throws ConnectionPoolException {
    //check for the first expired connection , close it and create a replacement
    //throw exception if replacement is not possible

    Entry&lt;JdbConnection, Instant&gt; entry =
        borrowed.entrySet().stream()
                        .filter(e -&gt; hasExpired(e.getValue()))
                        .findFirst()
                        .orElseThrow(() -&gt; new ConnectionPoolException("No connections available"));

    entry.getKey().close();
    borrowed.remove(entry.getKey());
    JdbConnection newJdbConnection = factory.create();
    borrowed.put(newJdbConnection,Instant.now());
    return newJdbConnection;
  }

  private boolean hasExpired(Instant instant) {
    return (Duration.between(instant, Instant.now()).toMillis() &gt; leaseTimeInMillis);
  }
}
I'm not exactly an expert in this field so perhaps someone else can provide a more in depth answer but here are my initial thoughts about this code:


  public void forfeit(JdbConnection jdbConnection) throws ConnectionPoolException {


Why do you need to burden the user with that exception? If the connection was already returned or forcibly taken, does the user of this method really care?
I would just silently check if that is the case and ignore the call if it is. 

thread safety?

Let's say we have a pool with only 1 connection left. What happens if 2 threads call the borrow()?

Thread one checks if (pool.size() &gt; 0) { and it passes. But now thread one is paused and thread two get's to run.

Thread two also checks if (pool.size() &gt; 0) { and it also passes.

Who will get the connection from the pool after this point?



It can actually get even worse. What if we not only have one connection available in the pool, but there's also an expired one.

Both threads should be able to get a connection right? But both went inside the if block. So 1 gets the connection, the other get's nothing.

The else part isn't executed for either of the threads.



Looking at the javadoc for ConcurrentLinkedQueue I don't think your code even works at the moment.

pool.remove() does not exist. pool.remove(Object obj) does but that returns a boolean to tell if removing the obj succeeded or not.

What you want is problably pool.poll().



How do you decide a good lease time? Sometimes a querry takes a couple of minutes to run. These are fine if you don't run them often. But if the lease time is set too low the query will never be able to finish. (Even tho the use case of that query could be a valid one).



Does factory.create() already set up an actual connection with the database? What if I set up a pool with 20 possible connections but actually only use a single connection for a couple of hours. Do I use up 20 connections with the database?

Why not create a new connection the moment one is asked (if we didn't pass the connection limit yet)?



What should I do if I didn't get a connection? (error no connection available). Should I run an active loop until I eventually get my connection?

You probably want to provide a method that puts the request into a queue (perhaps with a timeout) and gives the first available connection to the first in the queue. This way if too many threads are asking for a connection at once, they'll all eventually get their connection instead of overloading the system with busy-waiting-resource-wasting threads.





Multithreaded solutions are always tricky to get right. In general it's often better to try to search for an existing solution. Since I don't have experience in this field I can't really recommend any specific ones though...