commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [dbcp] 1.3 release problems
Date Mon, 23 Nov 2009 12:23:24 GMT
On 23/11/2009, Phil Steitz <phil.steitz@gmail.com> wrote:
> 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.

Yes, making protected fields private & final will change part of the API.

However, is that part of the API essential from the user point of view?
I.e. are there use-cases that require it?

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

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


Mime
View raw message