sebb wrote: > On 24/11/2009, sebb <seb...@gmail.com> wrote: >> On 24/11/2009, Phil Steitz <phil.ste...@gmail.com> wrote: >> > 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. >> >> >> Created dist/commons-dbpc.jar using "ant dist" under Java 1.6.0_17 (WinXP) >> >> ant -f test-jar.xml clean test -Dcp=dist/commons-dbcp.jar >> >> generates a lot of stack trace output but test succeeds. >> >> I then switched to Java 1.5.0_22 >> >> ant -f test-jar.xml clean test -Dcp=dist/commons-dbcp.jar >> >> fails with: >> >> compile-test: >> [mkdir] Created dir: >> D:\eclipseworkspaces\main\commons-dbcp-rw\build\test-classes >> [javac] Compiling 39 source files to >> D:\eclipseworkspaces\main\commons-dbcp-rw\build\test-classes >> [javac] >> D:\eclipseworkspaces\main\commons-dbcp-rw\build\src\test\org\apache\commons\dbcp\TestBasicDataSource.java:47: >> cannot access org.apache.com >> mons.dbcp.BasicDataSource >> [javac] bad class file: >> >> D:\eclipseworkspaces\main\commons-dbcp-rw\dist\commons-dbcp.jar(org/apache/commons/dbcp/BasicDataSource.class) >> [javac] class file has wrong version 50.0, should be 49.0 >> [javac] Please remove or make sure it appears in the correct >> subdirectory of the classpath. >> [javac] protected BasicDataSource ds = null; >> [javac] ^ >> [javac] 1 error >> >> This is presumably because the main build.xml file does not define the >> target Java version, so it defaults to 1.6. >> >> Or am I going about the test in the wrong way? >> >> I'll try changing the main build java target and see what happens. >> > > Using Java 1.6 to compile with target=1.5 works fine. > > However, the test fails for me with the same error when testing the > jar using Java 1.5. > > I think the error at the following line: > > 592: return new PoolableConnection(conn,_pool,_config); > > occurs because PoolableConnection extends DelegatingConnection which > imports java.sql.SQLClientInfoException (this is one of the imports > that are for JDBC4 only)
Thanks. That's the piece that I was missing. I conclude from this that we really are not going to be able to produce one binary jar that works for all supported JDKs. I will post a proposal shortly for packaging strategy. Thanks for getting the fixes in so the sources now compile clean for 1.6-1.6. Phil > > It's not possible to test on Java 1.4 at present because there is some > test code that requires Java 1.5 to build. > > Note that DBCP builds and tests OK for me using Java 1.5 throughout. > > >> > >> > 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 >> > >> > >> > > --------------------------------------------------------------------- > 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
