Phil Steitz wrote: > sebb wrote: >> On 22/11/2009, Phil Steitz <phil.ste...@gmail.com> 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. >> 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. > This ^^^ is bugging me as I don't see why it shouldn't work. I just committed an Ant build file, "test-jar.xml" that compiles and runs the tests against a compiled jar. Could be something wrong with my local setup, so I would appreciate it if others could test using JDK 1.5, 1.4.
Phil > > 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: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >>> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
