Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am looking for any feedback on my connection pool implementation (I removed the documentation so this post isn't so bloated). The pool times-out idle connections after a certain timeout interval and cleans itself after every 5 seconds (given that a method is called on it). There are no limits on pooled connections.

  • The Connection class is just a wrapper for a Socket or SocketChannel.
  • The Lang.close() method never throws any errors.

Is there any better way of doing this?

@ThreadSafe
public class StandardConnectionPool implements IConnectionPool {

    protected final HashMap<Domain, LinkedList<PooledConnection>> pooled;
    protected final long cleaning;
    protected final long timeout;
    protected long cleaned;

    public StandardConnectionPool(long timeout) throws AssertionError {
        assert timeout > 0l;

        synchronized (this) {
            this.pooled = new HashMap<>();
            this.cleaning = 5000;
            this.timeout = timeout;
            this.cleaned = System.currentTimeMillis();
        }
    }

    @Override
    public synchronized @Null Connection take(Domain domain) throws AssertionError {
        assert domain != null;

        this.clean();
        @Null LinkedList<PooledConnection> connections = this.pooled.get(domain);
        if (connections == null)
            return null;
        long threshold = System.currentTimeMillis() - this.timeout;
        PooledConnection connection;
        while ((connection = connections.poll()) != null) {
            if (connection.time >= threshold) {
                if (connections.size() == 0)
                    this.pooled.remove(domain);
                return connection.connection;
            }
            Lang.close(connection.connection);
        }
        if (connections.size() == 0)
            this.pooled.remove(domain);
        return null;
    }

    @Override
    public synchronized void pool(Domain domain, Connection connection) throws AssertionError {
        assert domain != null;
        assert connection != null;

        this.clean();
        @Null LinkedList<PooledConnection> connections = this.pooled.get(domain);
        if (connections != null) {
            connections.add(new PooledConnection(connection));
            return;
        }
        connections = new LinkedList<>();
        connections.add(new PooledConnection(connection));
        this.pooled.put(domain, connections);
    }

    protected synchronized void clean() {
        long time = System.currentTimeMillis();
        if (time < this.cleaned + this.cleaning)
            return;
        long threshold = time - this.timeout;
        Set<Entry<Domain, LinkedList<PooledConnection>>> set = this.pooled.entrySet();
        for (Entry<Domain, LinkedList<PooledConnection>> entry: set) {
            LinkedList<PooledConnection> connections = entry.getValue();
            Iterator<PooledConnection> iterator = connections.iterator();
            while (iterator.hasNext()) {
                PooledConnection connection = iterator.next();
                if (connection.time < threshold)
                    iterator.remove();
            }
            if (connections.size() == 0)
                set.remove(entry.getKey());
        }
    }

    @ThreadSafe
    protected class PooledConnection {

        public final long time;
        public final Connection connection;

        public PooledConnection(Connection connection) throws AssertionError {
            assert connection != null;

            this.time = System.currentTimeMillis();
            this.connection = connection;
        }

    }

}
share|improve this question

There is no need to add throws AssertionError to your methods signature - this is an unchecked exception.

You should mention any exceptions (checked and unchecked) in the javadoc though.

You abuse asserts - they are supposed to check things that can't happen, not to validate external arguments. You should replace them with more standard idioms, such as:

if (timeout <= 0) {
  throw new IllegalArgumentException("timeout must be positive (" + timeout + ")");
}
//...
Objects.requireNonNull(domain);
//etc.

Synchronizing within the constructor makes little sense - your object can only be constructed in one thread.

Your fields and one method are protected, meaning that one could extend the class and modify the map outside of a synchronized block (for example), breaking your synchronisation contract - this is possibly undesirable. It may be useful to (re-)read Effective Java #17: Design and document for inheritance or else prohibit it.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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