6
\$\begingroup\$

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<JdbConnection> pool = new ConcurrentLinkedQueue<>();

  private final Map<JdbConnection,Instant> borrowed = new ConcurrentHashMap<>();

  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 < 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() > 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<JdbConnection, Instant> entry =
        borrowed.entrySet().stream()
                        .filter(e -> hasExpired(e.getValue()))
                        .findFirst()
                        .orElseThrow(() -> 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() > leaseTimeInMillis);
  }
}
\$\endgroup\$
7
\$\begingroup\$

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() > 0) { and it passes. But now thread one is paused and thread two get's to run.

Thread two also checks if (pool.size() > 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...

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback Imus, gonna look at it. I recently changed the code to use ConcurrentLinkedQueue instead of just synchronizing the forfeit and borrow function, guess my unit tests were not good enough to detect concurrency issues after the refactor. \$\endgroup\$ – spakai Jun 21 '17 at 2:35
3
\$\begingroup\$

You have some concurrency problems. E.g. the ConcurrentHashMap, the java doc states for instance However, even though all operations are thread-safe, retrieval operations do not entail locking,.... It's like transactions on a database if you read committed: You only get what's done, you don't wait until other transactions have finished their work. I had a problem with this lately, there's a lot of information about that on stackoverflow.

I would get rid of the leaseTime. If one operation of an app can take hours, you have to either set hours for a leaseTime - which doesn't help very much, does it? -, or use different pools, which introduces more complexity the user usually doesn't want to be bothered with. But: It's the users task, to use his backend correctly.

When there's no connection left to borrow, the usualy behavior is to throw a "PoolExhaustedException". It's not the pool's task to be robust in the sense, that a user can do whatever he wants with the pool. On application servers, such as JBoss for instance, there's a message from the appl server, if the user forgets to close a connection ("closing connection for you"). The definition of the size of the pool is usually done by dev's, dba's, operations, qa and so on, at least for larger applications.

About the JdbcConnection. I'm also one of those guys who wraps abstraction layers around everything, but the java.sql API is not one of them, since it is an abstract layer itself. I'd recommend to go with the java.sql.Connection interface, but keep the JdbcConnectionFactory, so you can write your unit tests.

About your test cases:

If you use @Ignore annotation, always write down why it is ignored and if necessary/possible: Write what has to be done, to get rid of the annotation. Otherwise it will be remain there forever.

This line: ConnectionPool pool = new ConnectionPool(mockConnectionFactory, poolSize, shortLeaseLife); is redundant. Maybe create a newConnectionPool method with leaseTime as parameter (if you don't decide to get rid of it).

The simulateMultiplementClientsCalls should be re-written though: You should be able to test your concurrency without a thread pool of 50k or submitting 15k threads. But imo the wrong place to explain, since it's very complicated (or: I'm not very good at concurrency testing :P).

Hope this helps, slowy

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.