sebb wrote: > On 23/11/2009, Phil Steitz <phil.ste...@gmail.com> wrote: >> sebb wrote: >> > On 22/11/2009, Phil Steitz <phil.ste...@gmail.com> 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. >> > >> > 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. > > Yes, making protected fields private & final will change part of the API. > > However, is that part of the API essential from the user point of view? > I.e. are there use-cases that require it?
Not to my knowledge. Note that the same applies to lots of protected fields sprinkled throughout dbcp. This case is probably more ridiculous than others. From the ciirr report you can see that we have included a few similar breaks, so I am +0 on fixing this. Phil > >> >> > 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: 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 >> >> >> >> >> > >> > --------------------------------------------------------------------- >> > 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
