4
\$\begingroup\$

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

    }

}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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