commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sandy McArthur" <sandy...@apache.org>
Subject Re: [dbcp] Connection.close() implementation issues [was: Jumping in...]
Date Wed, 08 Mar 2006 19:15:00 GMT
On 3/8/06, Phil Steitz <phil.steitz@gmail.com> wrote:
> On 3/8/06, Sandy McArthur <sandymac@apache.org> wrote:
> > On 3/7/06, Phil Steitz <phil.steitz@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


Mime
View raw message