TCP connection pool

Question

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;
        }

    }

}

Answer 1

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.