You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@commons.apache.org by Sandy McArthur <sa...@gmail.com> on 2006/03/08 08:00:48 UTC
[dbcp] Connection.close() implementation issues [was: Jumping in...]
On 3/7/06, Phil Steitz <ph...@gmail.com> wrote:
> There is a growing backlog of bugs open against [dbcp]. Unless other
> committers object, I am going to jump in and start committing patches
> and develop a maintenance release plan. I plan to start with
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=33591
> http://issues.apache.org/bugzilla/show_bug.cgi?id=38073
>
> I would appreciate any feedback that community members have on
> prioritization of open tickets. Also, as always, tickets with patches
> and test cases are likely to get addressed quicker and comments /
> testing / feedback are always welcome.
>
> Any help from other commons committers, even if just in the form of
> screams, will be most appreciated.
I've looked into some, as you can tell from my retracted bugzilla
comment and I think I've found a large source of the problems.
The method Connection.close() interface states: "Calling the method
close on a Connection object that is already closed is a no-op."
This is violated in a number of places:
* PoolableConnection.java:77: throws an exception when close is called
and it's already closed. It should be replaced with a line similar to:
try {
if (!hasBeenReturnedToPool) {
hasBeenReturnedToPool = true;
_pool.invalidateObject(this);
}
} catch (Exception e) {
// ignored
}
* ConnectionImpl.java:115: calls assertOpen() which throws an
SQLException when it isn't open. This line should be removed.
* PoolingDataSource.java:179: the call to checkOpen() is like the call
to assertOpen() mentioned above and should be removed.
* PoolingDriver:258: another call to checkOpen(), same solution as before.
* TesterConnection.java:67: another call to checkOpen(), same solution
as before.
Someone should also audit:
* DelegatingConnection.java:184: make sure the call to passivate() is
fine being called more than once.
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-user-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-user-help@jakarta.apache.org
Re: [dbcp] Connection.close() implementation issues [was: Jumping in...]
Posted by Sandy McArthur <sa...@apache.org>.
On 3/8/06, Phil Steitz <ph...@gmail.com> wrote:
> On 3/8/06, Sandy McArthur <sa...@apache.org> wrote:
> > On 3/7/06, Phil Steitz <ph...@gmail.com> wrote:
> > > There is a growing backlog of bugs open against [dbcp]. Unless other
> > > committers object, I am going to jump in and start committing patches
> > > and develop a maintenance release plan. I plan to start with
> > >
> > > http://issues.apache.org/bugzilla/show_bug.cgi?id=33591
> > > http://issues.apache.org/bugzilla/show_bug.cgi?id=38073
> > >
> > > I would appreciate any feedback that community members have on
> > > prioritization of open tickets. Also, as always, tickets with patches
> > > and test cases are likely to get addressed quicker and comments /
> > > testing / feedback are always welcome.
> > >
> > > Any help from other commons committers, even if just in the form of
> > > screams, will be most appreciated.
> >
> > I've looked into some, as you can tell from my retracted bugzilla
> > comment and I think I've found a large source of the problems.
> >
> > The method Connection.close() interface states: "Calling the method
> > close on a Connection object that is already closed is a no-op."
>
> Unless I am misunderstanding the use case in the ticket, it seems to
> me that resolvoing it should lead us to qualify the above statement,
> not "make it true". We should search the archives for discussion on
> this, as I recall there was a discussion of the "no op" issue. IIUC,
> the problem in the ticket has to do with the corner case when the
> "already closed" state is entered without the knowledge of the pool.
I would be interested in reading the "no op" issue for Dbcp. Right
now I've only ever looked at Dbcp source code within the last 24 hours
so I cannot claim to be an expert on it.
My position on the "already closed" state is that since
PoolableConnection delegates to another Connection instance which in
this case may close unexpectedly it's reasonable for
PoolableConnection to delegate it's closed state to it's delegate
Connection.
> One other thing to keep in mind is that we should be aiming for a
> "dot-level" release (need to get a plan in place), so we are going to
> be constrained by backward compatibility and we have to be careful
> about changing behaviors that users may be counting on. Therefore, we
> should, wherever possible, apply the principle of least change when
> fixing bugs. Of course, that does not mean that we ignore significant
> issues or break contracts. Could be we what you are describing below
> is appropriate and necessary.
I'm fine with a band-aid dot-level release. I'm not yet ready to fully
tackle the Dbcp codebase. Since it seems a significant chunk of Dbcp
functionality is based on Pool functionality I want to mature that
some more first. After that I'll dive into Dbcp, try to load the code
into my head and how it's used and see what needs sorting out.
What code I have seen looks a bit like a patch work implemented by
different people with different ideas on how things should behave. I
only listed the close() methods that don't allow a closed Connection
to be closed again but there are others that looked like they were
no-ops on the 2nd call.
> Can you explain why the simple fixes in the ticket are not sufficient
> to close the issue?
The patch from Huw Lewis is basically one of the suggestions I made.
It has one problem of it will try to return the same object (this) to
the ObjectPool twice which breaks the ObjectPool contracts for the
expected life cycle of a borrowed object. That's why I suggested the
if (!hasBeenReturnedToPool) logic.
I think the close() methods in general should be more robust and less
Exception prone. Since it's rather common for a Connection to delegate
to another Connection class, like PoolableConnection.close() does, I
don't think it's right to just cover up the mess at PC.close() and
ignore deeper issues. But that can wait until a more significant
release, IMO.
> Thanks for looking into this.
>
> Phil
>
> >
> > This is violated in a number of places:
> > * PoolableConnection.java:77: throws an exception when close is called
> > and it's already closed. It should be replaced with a line similar to:
> > try {
> > if (!hasBeenReturnedToPool) {
> > hasBeenReturnedToPool = true;
> > _pool.invalidateObject(this);
> > }
> > } catch (Exception e) {
> > // ignored
> > }
> >
> > * ConnectionImpl.java:115: calls assertOpen() which throws an
> > SQLException when it isn't open. This line should be removed.
> >
> > * PoolingDataSource.java:179: the call to checkOpen() is like the call
> > to assertOpen() mentioned above and should be removed.
> >
> > * PoolingDriver:258: another call to checkOpen(), same solution as before.
> >
> > * TesterConnection.java:67: another call to checkOpen(), same solution
> > as before.
> >
> > Someone should also audit:
> > * DelegatingConnection.java:184: make sure the call to passivate() is
> > fine being called more than once.
> >
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [dbcp] Connection.close() implementation issues [was: Jumping in...]
Posted by Phil Steitz <ph...@gmail.com>.
On 3/8/06, Sandy McArthur <sa...@apache.org> wrote:
> Sorry, ment to send this to commons-dev, not commons-user.
> /me needs sleep asap.
>
> ---------- Forwarded message ----------
> From: Sandy McArthur <sa...@gmail.com>
> Date: Mar 8, 2006 2:00 AM
> Subject: [dbcp] Connection.close() implementation issues [was: Jumping in...]
> To: Jakarta Commons Users List <co...@jakarta.apache.org>
>
>
> On 3/7/06, Phil Steitz <ph...@gmail.com> wrote:
> > There is a growing backlog of bugs open against [dbcp]. Unless other
> > committers object, I am going to jump in and start committing patches
> > and develop a maintenance release plan. I plan to start with
> >
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=33591
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=38073
> >
> > I would appreciate any feedback that community members have on
> > prioritization of open tickets. Also, as always, tickets with patches
> > and test cases are likely to get addressed quicker and comments /
> > testing / feedback are always welcome.
> >
> > Any help from other commons committers, even if just in the form of
> > screams, will be most appreciated.
>
> I've looked into some, as you can tell from my retracted bugzilla
> comment and I think I've found a large source of the problems.
>
> The method Connection.close() interface states: "Calling the method
> close on a Connection object that is already closed is a no-op."
Unless I am misunderstanding the use case in the ticket, it seems to
me that resolvoing it should lead us to qualify the above statement,
not "make it true". We should search the archives for discussion on
this, as I recall there was a discussion of the "no op" issue. IIUC,
the problem in the ticket has to do with the corner case when the
"already closed" state is entered without the knowledge of the pool.
One other thing to keep in mind is that we should be aiming for a
"dot-level" release (need to get a plan in place), so we are going to
be constrained by backward compatibility and we have to be careful
about changing behaviors that users may be counting on. Therefore, we
should, wherever possible, apply the principle of least change when
fixing bugs. Of course, that does not mean that we ignore significant
issues or break contracts. Could be we what you are describing below
is appropriate and necessary.
Can you explain why the simple fixes in the ticket are not sufficient
to close the issue?
Thanks for looking into this.
Phil
>
> This is violated in a number of places:
> * PoolableConnection.java:77: throws an exception when close is called
> and it's already closed. It should be replaced with a line similar to:
> try {
> if (!hasBeenReturnedToPool) {
> hasBeenReturnedToPool = true;
> _pool.invalidateObject(this);
> }
> } catch (Exception e) {
> // ignored
> }
>
> * ConnectionImpl.java:115: calls assertOpen() which throws an
> SQLException when it isn't open. This line should be removed.
>
> * PoolingDataSource.java:179: the call to checkOpen() is like the call
> to assertOpen() mentioned above and should be removed.
>
> * PoolingDriver:258: another call to checkOpen(), same solution as before.
>
> * TesterConnection.java:67: another call to checkOpen(), same solution
> as before.
>
> Someone should also audit:
> * DelegatingConnection.java:184: make sure the call to passivate() is
> fine being called more than once.
>
> --
> Sandy McArthur
>
> "He who dares not offend cannot be honest."
> - Thomas Paine
>
>
> --
> Sandy McArthur
>
> "He who dares not offend cannot be honest."
> - Thomas Paine
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
[dbcp] Connection.close() implementation issues [was: Jumping in...]
Posted by Sandy McArthur <sa...@apache.org>.
Sorry, ment to send this to commons-dev, not commons-user.
/me needs sleep asap.
---------- Forwarded message ----------
From: Sandy McArthur <sa...@gmail.com>
Date: Mar 8, 2006 2:00 AM
Subject: [dbcp] Connection.close() implementation issues [was: Jumping in...]
To: Jakarta Commons Users List <co...@jakarta.apache.org>
On 3/7/06, Phil Steitz <ph...@gmail.com> wrote:
> There is a growing backlog of bugs open against [dbcp]. Unless other
> committers object, I am going to jump in and start committing patches
> and develop a maintenance release plan. I plan to start with
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=33591
> http://issues.apache.org/bugzilla/show_bug.cgi?id=38073
>
> I would appreciate any feedback that community members have on
> prioritization of open tickets. Also, as always, tickets with patches
> and test cases are likely to get addressed quicker and comments /
> testing / feedback are always welcome.
>
> Any help from other commons committers, even if just in the form of
> screams, will be most appreciated.
I've looked into some, as you can tell from my retracted bugzilla
comment and I think I've found a large source of the problems.
The method Connection.close() interface states: "Calling the method
close on a Connection object that is already closed is a no-op."
This is violated in a number of places:
* PoolableConnection.java:77: throws an exception when close is called
and it's already closed. It should be replaced with a line similar to:
try {
if (!hasBeenReturnedToPool) {
hasBeenReturnedToPool = true;
_pool.invalidateObject(this);
}
} catch (Exception e) {
// ignored
}
* ConnectionImpl.java:115: calls assertOpen() which throws an
SQLException when it isn't open. This line should be removed.
* PoolingDataSource.java:179: the call to checkOpen() is like the call
to assertOpen() mentioned above and should be removed.
* PoolingDriver:258: another call to checkOpen(), same solution as before.
* TesterConnection.java:67: another call to checkOpen(), same solution
as before.
Someone should also audit:
* DelegatingConnection.java:184: make sure the call to passivate() is
fine being called more than once.
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
--
Sandy McArthur
"He who dares not offend cannot be honest."
- Thomas Paine
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org