sebb wrote: > 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?
No and at this point it is very hard to tell where to draw the line on what *should* be public. > > 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. Could be a good idea for 2.0, when we can have a robust discussion on what should be public. Phil > >> 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
