commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phil Steitz (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (DBCP-398) DBCP hangs on common pool borrowObject when PoolableConnection is used and the underlying connection closed unexpectedly (connection resets/timouts)
Date Fri, 24 May 2013 15:02:20 GMT

    [ https://issues.apache.org/jira/browse/DBCP-398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13666376#comment-13666376
] 

Phil Steitz edited comment on DBCP-398 at 5/24/13 3:02 PM:
-----------------------------------------------------------

Thanks, Sarvesh. Looking at the client code, I now understand what is going on. You are using
isClosed() to decide whether or not to close (i.e. return to the pool) the PoolableConnection.
 Sorry for being a little slow. I agree that the isClosed implementation in PoolableConnection
is questionable, but it arguably conforms to the JDBC spec (closed means it can no longer
be used to connect to the database) and I wonder what other client apps might break if we
change it.  I am on the fence and would appreciate some more community input on whether or
not we should make this change, which is a basically a change in the contract for PoolableConnection#isClosed.
 It could be that it is best to make this change in 2.0 (which has the same behavior) or via
a new method (see below).  Arguably, the client code should not be testing isClosed, but just
invoking close directly.  As of 1.3/1.4, close is idempotent, so there is no risk of exception
on multiple close.  In any case, once a connection has been returned to the pool, clients
should discard references to it.

What do others think about the proposed changes in behavior:
 
(1) drop _con.isClosed() test from PoolableConnection#isClosed() (so "closed" really means
"has been returned to the pool") 
(2) do not throw when a connection that has been closed without knowledge of the pool is closed.

One more note on this. If we do decide to make change (1), it might be better to implement
it in the PoolGuardConnectionWrapper, possibly even implementing a new method, isDiscarded()
or somesuch to disambiguate isClosed.
                
      was (Author: psteitz):
    Thanks, Sarvesh. Looking at the client code, I now understand what is going on. You are
using isClosed() to decide whether or not to close (i.e. return to the pool) the PoolableConnection.
 Sorry for being a little slow. I agree that the isClosed implementation in PoolableConnection
is questionable, but it arguably conforms to the JDBC spec (closed means it can no longer
be used to connect to the database) and I wonder what other client apps might break if we
change it.  I am on the fence and would appreciate some more community input on whether or
not we should make this change, which is a basically a change in the contract for PoolableConnection#isClosed.
 It could be that it is best to make this change in 2.0 (which has the same behavior) or via
a new method (see below).  Arguably, the client code should not be testing isClosed, but just
invoking close directly.  As of 1.3/1.4, close is idempotent, so there is no risk of exception
on multiple close.  In any case, once a connection has been returned to the pool, clients
should discard references to it.

What do others think about the proposed changes in behavior:
 
(1) drop _con.isClosed() test from PoolableConnection#isClosed() (so "closed" really means
"has been returned to the pool") 
(2) do not throw when a connection that has been closed without knowledge of the pool is closed.

One more note on this. If we do decide to make a change, it might be better to implement it
in the PoolGuardConnectionWrapper, possibly even implementing a new method, isDiscarded()
or somesuch to disambiguate isClosed.
                  
> DBCP hangs on common pool borrowObject when PoolableConnection is used and the underlying
connection closed unexpectedly (connection resets/timouts)
> ----------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: DBCP-398
>                 URL: https://issues.apache.org/jira/browse/DBCP-398
>             Project: Commons Dbcp
>          Issue Type: Bug
>    Affects Versions: 1.2, 1.3, 1.4, 2.0
>            Reporter: Sarvesh Sakalanaga
>         Attachments: DBCP-398.0.patch
>
>
> The bug is in org.apache.commons.dbcp.PoolableConnection as isClosed method on this calls
super.isClosed which returns true (as DelegatingConnection::isClosed { _closed || _conn.isClosed()
}). Since PoolableConnection needs to release objects to pool even if the underlying connection
is closed the isClosed method should be overridden in this class and should return _closed.
This _closed is the delegating connection close which will be set to false even if the underlying
connection is closed (_conn.isClosed). The fix should also not throw on PoolableConnection::Close
method if underlying connection is closed as this state is a valid state and is expected.
> Also currently the way it stands the clients of PoolableConnection will/may not call
Close() as isClosed always returns true in this case.
> Below is the stack that the thread hangs on:
> ◾waiting on <0x00000007b5a50e48> (a org.apache.commons.pool.impl.GenericObjectPool$Latch)
>  at java.lang.Object.wait(Object.java:503)
>  at org.apache.commons.pool.impl.GenericObjectPool.borrowObject(GenericObjectPool.java:1104)
> ◾locked <0x00000007b5a50e48> (a org.apache.commons.pool.impl.GenericObjectPool$Latch)
>  at org.apache.commons.dbcp.PoolingDataSource.getConnection(PoolingDataSource.java:106)
>  at org.datanucleus.store.rdbms.ConnectionProviderPriorityList.getConnection(ConnectionProviderPriorityList.java:57)
>  at org.datanucleus.store.rdbms.ConnectionFactoryImpl$ManagedConnectionImpl.getConnection(ConnectionFactoryImpl.java:354)
>  at org.datanucleus.store.rdbms.ConnectionFactoryImpl$ManagedConnectionImpl.getXAResource(ConnectionFactoryImpl.java:314)
>  at org.datanucleus.store.connection.ConnectionManagerImpl.enlistResource(ConnectionManagerImpl.java:386)
>  at org.datanucleus.store.connection.ConnectionManagerImpl.allocateConnection(ConnectionManagerImpl.java:252)
>  at org.datanucleus.store.connection.AbstractConnectionFactory.getConnection(AbstractConnectionFactory.java:60)
>  at org.datanucleus.store.AbstractStoreManager.getConnection(AbstractStoreManager.java:449)
>  at org.datanucleus.store.AbstractStoreManager.getConnection(AbstractStoreManager.java:418)
>  at org.datanucleus.store.rdbms.query.JDOQLQuery.performExecute(JDOQLQuery.java:595)

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message