On 24/11/2009, Phil Steitz <phil.ste...@gmail.com> wrote: > 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.
If we could define which methods are part of the public API, and ensure that their classes don't need any conditional compilation, then it might be possible to sort it out so the appropriate versions of the other classes are loaded at run-time. Or perhaps we could use reflection to invoke methods that are new in JDBC4, or compile against JDBC4, and catch the ClassNotFound errors when using JDBC3. It would be helpful if there were some unit tests that just exercised the public API - maybe the examples could be turned into tests? Do the examples exercise all the public API? Maybe consider moving the internal classes to a .internal package (as suggested for JEXL); if they are internal then it does not matter breaking the API. > 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. 1.4-1.6 ;-) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
