Hi Paul
I was going have a look at the background thread for closing unused
connections, but your rock at this! Could you maybe have a look at it since
you're already doing work on the class.
~ Leon
Paul O'Leary wrote:
> 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]
>
------------------------------------------------------------
To subscribe: [EMAIL PROTECTED]
To unsubscribe: [EMAIL PROTECTED]
Search: <http://www.mail-archive.com/turbine%40list.working-dogs.com/>
Problems?: [EMAIL PROTECTED]
