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.
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
