sebb wrote: > On 22/11/2009, Phil Steitz <[email protected]> wrote: >> sebb wrote: >> > On 22/11/2009, Phil Steitz <[email protected]> wrote: >> >> I am running into some problems preparing for dbcp-1.3. I would >> >> appreciate comments / patches on any of the issues below. >> >> >> >> 1. Findbugs is showing some real (inconsistent synch) and not so >> >> real (e.g. serialization issues on classes that IMO should not be >> >> serializable, but we can't fix until 2.0). The full report is here: >> >> http://commons.apache.org/dbcp/findbugs.html >> >> I would appreciate suggestions/patches/commits for what to fix and how. >> > >> > org.apache.commons.dbcp.AbandonedTrace$AbandonedObjectException.format >> > - not a problem, as the code is synch. on format, just disable the report >> >> >> +1 >> >> > >> org.apache.commons.dbcp.PoolableConnectionFactory._connFactory,_pool,_validationQuery >> > => just make these volatile. >> >> >> +1 - all we can do without breaking compat >> >> > org.apache.commons.dbcp.PoolingConnection.createKey(String, byte) >> > might ignore java.lang.Exception (lines218, 229, 240 and 251) >> > No idea >> >> >> This is silly - exceptions potentially thrown by getCatalog are >> (intentionally) swallowed. > > However the methods should only catch SQLException, not Exception.
+1 - and that we can fix. > > _catalog should probably have been final and private. Or could > probably be dropped altogether, as it does not seem to be necessary. I think the field is necessary - at least there was a BZ ticket that caused it to be added ([Bug 27246] - PreparedStatement cache should be different depending on the Catalog). Agree it and other key fields should be final. Unfortunately, they are all protected now, so to fix is to break. > >> > PoolingConnection$PStmtKey.PoolingConnection$PStmtKey._resultSetType >> > could be null and is guaranteed to be dereferenced in >> > org.apache.commons.dbcp.PoolingConnection.makeObject(Object) >> > This looks like a bug; just check for null in the second condition? >> >> >> Should never happen, but will refactor to explicitly avoid. >> >> > Class org.apache.commons.dbcp.cpdsadapter.DriverAdapterCPDS defines >> > non-transient non-serializable instance field logWriter >> > Just make the logWriter transient. >> >> >> +1 >> >> > _pool synch: add synch or make volatile. >> >> >> I guess make volatile is safest. >> >>> <aside> >> > Seems to me a lot of these synch. problems would be avoided if the >> > variables did not have set() methods - why are there set() methods for >> > fields that are provided in the constructors? What is the use case for >> > this? >> > </aside> >> >> >> Agree strongly with comment. BasicDataSource is crippled by this. >> It is effectively immutable once getConnection has been called, but >> the public setters and protected fields make it impossible to fix >> without breaking compatibility. See DBCP-300 for example of how >> this causes needless performance problems. For Tomcat, I have been >> thinking about providing an alternative JNDI factory that returns a >> PoolingDataSource instead. >> >> >> > >> > It would be helpful to know which classes are intended to be >> > thread-safe, as it's not clear whether the potential synch. problems >> > are likely to occur in normal usage or not. >> > >> > For example the class SharedPoolDataSource: the field "pool" is >> > sometimes synch., and sometimes not, but the fields maxActive, >> > maxWait, maxIdle are not synch. at all. >> >> >> Here again, all of these should be immutable properties set by the >> constructor. >> >> > The use of synchronization seems rather haphazard to me. >> >> >> harsh but true ;) Comment above really covers it - the needlessly >> sloppy synch is in most cases due to overly mutable - and sometimes >> directly exposed - properties and no concern for synch issues that >> are not likely to occur in normal use. I am +1 for fixing anything >> that we can pre-2.0 subject to compat and performance constraints. >> >> >> 2. We can't compile commons-pool-1.3.jar against JDK 1.6 (JDBC 4) >> >> and expect it to work for JDK 1.4/1.5 (JDBC 3) clients (at least not >> >> as the code stands today). So we need to create two jar artifacts. >> > >> > How difficult would it be to support both in the same jar? >> >> >> I would like to do that if we could do it safely. I have not been >> able to get the 1.6-compiled jar to successfully run the tests >> compiled against 1.5. >> >> The failure that I get when using Ant to compile and execute the >> tests (commenting out the 1.6-stuff in the test classes) using a >> 1.6-built jar is strange: >> >> [junit] Exception in thread "Thread-16" >> java.lang.NoClassDefFoundError: java/sql/SQLClientInfoException >> [junit] at >> >> org.apache.commons.dbcp.PoolableConnectionFactory.makeObject(PoolableConnectionFactory.java:592) >> [junit] at >> >> org.apache.commons.dbcp.BasicDataSource.validateConnectionFactory(BasicDataSource.java:1537) >> [junit] at >> >> org.apache.commons.dbcp.BasicDataSource.createPoolableConnectionFactory(BasicDataSource.java:1526) >> [junit] at >> >> org.apache.commons.dbcp.BasicDataSource.createDataSource(BasicDataSource.java:1374) >> [junit] at >> >> org.apache.commons.dbcp.BasicDataSource.getConnection(BasicDataSource.java:1038) >> [junit] at >> >> org.apache.commons.dbcp.TestBasicDataSource.getConnection(TestBasicDataSource.java:44) >> [junit] at >> >> org.apache.commons.dbcp.TestConnectionPool.newConnection(TestConnectionPool.java:84) >> [junit] at >> >> org.apache.commons.dbcp.TestConnectionPool$TestThread.run(TestConnectionPool.java:595) >> [junit] at java.lang.Thread.run(Thread.java:613) >> >> Strange as the line number in PCF.makeObject and missing class makes >> no sense. >> >> >> Thanks, Sebb! >> >> >> The question is which one gets the 1.3 name, what is the other >> >> named and how do we package the distros? >> >> >> >> 3. I assume it is OK at this point to drop the nojdbc3 Ant target >> >> and compiler flags for JDBC 2. >> >> >> >> TIA >> >> >> >> Phil >> >> >> >> --------------------------------------------------------------------- >> >> To unsubscribe, e-mail: [email protected] >> >> For additional commands, e-mail: [email protected] >> >> >> >> >> > >> > --------------------------------------------------------------------- >> > To unsubscribe, e-mail: [email protected] >> > For additional commands, e-mail: [email protected] >> > >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
