Paul, thank you for your excellent patch. It is now in CVS. My patch
binary had some trouble with the first hunk, so if you would CVS update
and make sure everything is kosher, I'd appreciate it.
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]
--
Daniel Rall <[EMAIL PROTECTED]>
------------------------------------------------------------
To subscribe: [EMAIL PROTECTED]
To unsubscribe: [EMAIL PROTECTED]
Search: <http://www.mail-archive.com/turbine%40list.working-dogs.com/>
Problems?: [EMAIL PROTECTED]
