Hi all,
I spent some time today going through the connection pool code. I remember
Jon once rhetorically asking 'what was wrong with Turbine's connection
pool'? This is my answer ;) I have a lot of DB and concurrency experience
so it seemed like low hanging fruit.
In general Turbine's connection pool is lightweight and simple - a good
thing. There seems to be a couple simple thread safety issues in DBBroker
for which I will append a patch. There are some more general
concurrency/safety/design issues that are open for debate. Here are my
comments, in no particular order:
In ConnectionPool.java:
. getConnection( ... ) is synched; this has the effect of serializing access
to *all* connections to a particular database type.
one effect of this may be good: serialized connects guard against driver
problems with concurrent connections. my experience is that many JDBC and
ODBC (if you're using a bridge) drivers are not thread-safe during connect.
another affect may be bad: blocks *all* access to the connection pool while
a user is connecting. scenario: request for connection finds none
available, goes to allocate and connect new one - this can take a while e.g.
i've seen oracle take up to 10 seconds to connect. no connections can be
put back in pool while this is happening - releaseConnection is synched as
well - so other requests will block.
. usually a bad idea to put cleanup code in finalize; totally undefined as
to when this is called in the life of the server. might just wanna throw
exception if the pool is non-empty - this signifies bad shutdown.
. seems like it might be a bad idea to wait() in getPooledConnection( ... ).
this could produce a hard-to-diagnose performance problem because you might
not be allowing Turbine to allocate enough connections but the system still
seems to run fine. might want to throw exception (not enough connections)
or at least log something if waits are happening - this tells you you need
to increase connections.
. defining a connection manager where connections are 'popped' out and have
to be released back can be error-prone.
no safety checks in case bad code doesn't release the connections. might
want to introduce another hashtable that keeps track of 'checked out'
connections - at lease for error checking/debugging purposes. checked out
connections can be tagged with session info; thread that checks for
staleness could also check that there are no orphaned connections.
not a huge deal because in practice if there's bad code you'll just run out
of db connections real fast. this compounds
the wait() problem noted above, though.
. no checking for stale connections currently implemented.
In DBBroker.java:
. getConnection( ... ): hashtable access not synched. see patch.
. throws *Exception* base class - yucko! explicit error handling, please!
. add/getDatabaseMap et al: sync problems. see patch.
. getDB( name ) is weird code. is it thread-safe?
OVERALL:
. cache basically 'pushes' and 'pops' DBConnections with no 'sharing' of
connections; that is, two sessions using the same connection for DB access.
this is fine but can be inefficient by some standards especially if quite a
few connections are used 'read only' and can be shared. this is
particularly a problem with databases like Oracle where connections are
pretty heavy-weight. however, since 'writable' connections are a priority
here it can be complicated to introduce caching for both readable and
writable. users must ask for whichever they want and code accordingly.
could put extra logic in DBConnection (start trans/commit?) to validate if
we wanted to do this.
. poor exception handling in some cases: every method that handles
exceptions seems to catch Exception...? Methods should only throw explicit
exceptions!
Hope there's some other propeller heads out there like me who find this
stuff interesting. I've implemented and maintainted quite a few
closed-source connection pools, some more complicated than others; I'd love
to see just *one* open source pool that gets it all right so I never have to
do it again!
I'll append my patch to DBBroker.java. Please treat these changes as
proposed - I'm not set up to test them right now. I'd also appreciate
immediate - but gentle :) - feedback as to whether the patch formatting is
appropriate, useable, etc.
cheers,
PaulO.
---
Index: DBBroker.java
===================================================================
RCS file:
/products/cvs/turbine/turbine/src/java/org/apache/turbine/util/db/pool/DBBro
ker.java,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 DBBroker.java
--- DBBroker.java 2000/07/19 02:57:33 1.1.1.1
+++ DBBroker.java 2000/08/28 23:56:10
@@ -146,18 +146,27 @@
String username,
String password) throws Exception
{
- ConnectionPool pool;
+ ConnectionPool pool = null;
url = url.trim();
- if ( pools.containsKey(url) )
+ // quick (non-sync) check for the pool we want
+ pool = (ConnectionPool) pools.get(url);
+ if ( pool == null )
{
- pool = (ConnectionPool) pools.get(url);
- }
- else
- {
- pool = new ConnectionPool();
- pools.put( url, pool );
+ // pool not there...
+ synchronized ( pools )
+ {
+ // ...sync and look again to avoid race collisions
+ pool = (ConnectionPool) pools.get(url);
+ if ( pool == null )
+ {
+ // still not there. create and add.
+ pool = new ConnectionPool();
+ pools.put( url, pool );
+ }
+ }
}
+
return pool.getConnection(driver, url, username, password);
}
@@ -179,16 +188,27 @@
if ( name == null )
throw new Exception ("DatabaseMap name was null!");
- if ( dbMaps.containsKey(name) )
- {
- return (DatabaseMap)dbMaps.get(name);
- }
- else
+ DatabaseMap map = null;
+
+ // quick (non-sync) check for the map we want
+ map = (DatabaseMap)dbMaps.get(name);
+ if ( map == null )
{
- DatabaseMap dbMap = new DatabaseMap(name);
- dbMaps.put(name, dbMap);
- return dbMap;
+ // map not there...
+ synchronized( dbMaps )
+ {
+ // ... sync and look again
+ map = (DatabaseMap)dbMaps.get(name);
+ if ( map == null )
+ {
+ // still not there. create and add.
+ map = new DatabaseMap(name);
+ dbMaps.put(name, map);
+ }
+ }
}
+
+ return map;
}
/**
@@ -198,7 +218,10 @@
*/
public void addDatabaseMap(String name)
{
- dbMaps.put(name, new DatabaseMap(name) );
+ synchronized( dbMaps )
+ {
+ dbMaps.put(name, new DatabaseMap(name) );
+ }
}
/**
@@ -209,7 +232,10 @@
*/
public void addDatabaseMap(DatabaseMap map)
{
- dbMaps.put (map.getName(), map);
+ synchronized( dbMaps )
+ {
+ dbMaps.put (map.getName(), map);
+ }
}
/**
* Returns the database specific class.
@@ -263,11 +289,13 @@
String url = connection.getUrl();
if ( url != null )
{
- if (pools.containsKey( url ))
+ ConnectionPool pool = null;
+ synchronized ( pools )
{
- ConnectionPool pool = (ConnectionPool)
pools.get( url );
- pool.releaseConnection( connection );
+ pool = (ConnectionPool) pools.get( url );
}
+ if ( pool != null )
+ pool.releaseConnection( connection );
// else if there is no pool for this connection
// don't do anything
}
------------------------------------------------------------
To subscribe: [EMAIL PROTECTED]
To unsubscribe: [EMAIL PROTECTED]
Search: <http://www.mail-archive.com/turbine%40list.working-dogs.com/>
Problems?: [EMAIL PROTECTED]
