Phil Steitz wrote:
> sebb wrote:
>> 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.
The changes below will make Findbugs happy, but will have a slight
performance impact. Since RTE is thrown in either case, I am
inclined to leave as is (since path should in fact never get executed).
---
src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java
(revision 883502)
+++
src/java/org/apache/commons/dbcp/cpdsadapter/PooledConnectionImpl.java
(working copy)
@@ -404,7 +404,7 @@
*/
public Object makeObject(Object obj) throws Exception {
if (null == obj || !(obj instanceof PStmtKey)) {
- throw new IllegalArgumentException();
+ throw new IllegalArgumentException("Invalid or null
statement key");
} else {
// _openPstmts++;
PStmtKey key = (PStmtKey)obj;
@@ -421,11 +421,15 @@
key, pstmtPool, delegatingConnection);
}
} else {
- return new PoolablePreparedStatementStub(
- connection.prepareStatement(key._sql,
- key._resultSetType.intValue(),
- key._resultSetConcurrency.intValue()),
- key, pstmtPool, delegatingConnection);
+ if (null == key._resultSetType || null ==
key._resultSetConcurrency) {
+ throw new IllegalArgumentException("Invalid
statement key");
+ } else {
+ return new PoolablePreparedStatementStub(
+ connection.prepareStatement(key._sql,
+ key._resultSetType.intValue(),
+
key._resultSetConcurrency.intValue()),
+ key, pstmtPool,
delegatingConnection);
+ }
}
}
}
>> 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
