commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [dbcp] 1.3 release problems
Date Mon, 23 Nov 2009 04:41:40 GMT
sebb wrote:
> On 22/11/2009, Phil Steitz <phil.steitz@gmail.com> wrote:
>> sebb wrote:
>>  > On 22/11/2009, Phil Steitz <phil.steitz@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.
> 
>>  > 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-unsubscribe@commons.apache.org
>>  >>  For additional commands, e-mail: dev-help@commons.apache.org
>>  >>
>>  >>
>>  >
>>  > ---------------------------------------------------------------------
>>  > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>  > For additional commands, e-mail: dev-help@commons.apache.org
>>  >
>>
>>
>>  ---------------------------------------------------------------------
>>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>  For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message