patternjavaMinor
Connection pool implementation
Viewed 0 times
poolimplementationconnection
Problem
I wanted to try out writing an minimalistic connection pool implementation (out of curiosity and for learning). Could you all please review the code and provide feedback?
There are two classes (class names are hyperlinked to code):
TODOs (based on review comments):
This is why code reviews are important. I've added the following changes so far based on the code review comments:
The code:
```
package com.amitcodes.dbcp;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.logging.Level;
import java.util.logging.Logger;
public class ConnectionPool {
private static final Logger logger = Logger.getLogger(ConnectionPool.class.getCanonicalName());
private BlockingQueue pool;
/**
* Maximum number of connections that the pool can have
*/
private int maxPoolSize;
/**
* Number of connections that should be created initially
*/
private int ini
There are two classes (class names are hyperlinked to code):
com.amitcodes.dbcp.ConnectionPool: The connection pool implementation
com.amitcodes.dbcp.PooledConnection: The proxy forjava.sql.Connection, written with the intent of ensuring that connections borrowed fromConnectionPoolshould be not be closed by client code, but surrendered back to the pool.
TODOs (based on review comments):
This is why code reviews are important. I've added the following changes so far based on the code review comments:
- maintain a count of active connections available in the pool (preferably using
AtomicInteger)
borrowConnection()should ensure there are no available idle connections (using point #1 above) before opening a new pooled connection
- surrenderConnection should rollback unfinished transactions. They could leak.
- validate that surrendered connections are same as ones which were borrowed. If this check is not in place, a client can surrender connection to db 'foo' to dbcp for db 'bar'
surrenderConnection()should take care of rolling back any open transactions
- include a validate() method to validate constructor params
The code:
```
package com.amitcodes.dbcp;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.logging.Level;
import java.util.logging.Logger;
public class ConnectionPool {
private static final Logger logger = Logger.getLogger(ConnectionPool.class.getCanonicalName());
private BlockingQueue pool;
/**
* Maximum number of connections that the pool can have
*/
private int maxPoolSize;
/**
* Number of connections that should be created initially
*/
private int ini
Solution
A few random notes:
-
If I were you I'd check the source and API of Apache Commons Pool. You could borrow ideas about possible errors, corner cases, useful API calls, parameters etc.
-
-
SLF4J has better API than JUL. (Check Logback also, if you don't familiar with it.)
-
A malicious (or poorly written client with multiple pools) can call
-
I'd check
(Also see: Effective Java, 2nd edition, Item 38: Check parameters for validity)
-
If I were you I'd check the source and API of Apache Commons Pool. You could borrow ideas about possible errors, corner cases, useful API calls, parameters etc.
-
surrenderConnection should rollback unfinished transactions. They could leak.-
SLF4J has better API than JUL. (Check Logback also, if you don't familiar with it.)
-
A malicious (or poorly written client with multiple pools) can call
surrenderConnection with a PooledConnection instance which was not created by the called ConnectionPool.-
I'd check
nulls in the constructors/methods. Does it make sense to call them with null? If not, check it and throw an exception. checkNotNull in Guava is a great choice for that.this.connectionPool =
checkNotNull(connectionPool, "connectionPool cannot be null");(Also see: Effective Java, 2nd edition, Item 38: Check parameters for validity)
Code Snippets
this.connectionPool =
checkNotNull(connectionPool, "connectionPool cannot be null");Context
StackExchange Code Review Q#40005, answer score: 9
Revisions (0)
No revisions yet.