From: Warren Young
Date: December 27 2007 3:41am
Subject: Re: problems in ConnectionPool
List-Archive: http://lists.mysql.com/plusplus/7306
Message-Id: <47731EF3.1030601@etr-usa.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Jonathan Wakely wrote:
> There are some problems with ConnectionPool:

Yeah, that can happen when you release a class without ever actually 
using it. ;)  Actual use is waiting on me to get around to writing the 
"threading" example program mentioned in the Wishlist.

Do you have a working pool subclass, or are these patches being done in 
vitro, the same way the class was created?

I've checked in a change based on your patch, with quite a lot of 
changes, and not just for style.  Comments in line below:

> I have a work-in-progress which uses a functor and
> std::for_each (or std::max_element) to iterate over the list,
> remembering the best one to return, and a list of old connections that
> should be freed after the loop finishes.

Yes, I've failed to make use of STL algorithms habitual... :(

I changed the loop structures for style and clarity reasons.  You might 
recheck them in case I broke their function somehow.

> 3) last_used never seems to be updated.  The patch sets last_used to
> time(0) when it is returned to the pool in release(), 

FYI, this wasn't done in the second patch, which overlaps the first. 
Because both wouldn't apply at the same time, I just applied the second 
and then worked with the resulting files and have done little but glance 
at the first.  If there's something missing, this is why.

> I'm not sure if
> that's better than setting last_used in grab() instead.

Resetting the time in release() is the right thing.

The implication on release() is that the connection was used just prior 
to being checked back into the pool.  That's the best the pool can do 
without explicit cooperation from Connection, and that's a pretty poor 
reason to couple these classes.

I documented this is the Doxygen comment for release().  The comments in 
the coming threading example will also make this clear.

> I've also added a simple RAII class for locking/unlocking the mutex in
> an exception-safe way.

Yes, good.  I moved it to the same file as BeecryptMutex, though.

> This means the derived destructor is required to destroy everything in
> pool_, so this revised patch adds a clear_pool() member which derived
> classes must call before the base ~ConnectionPool dtor runs.

Yes, that's correct.

I renamed the method to clear() to match STL.  I also considered 
drain(), but decided that was just a bit too cutesy. :)

I don't have a good reason for an outsider to drain a pool, but I also 
don't have one for why an outsider shouldn't be allowed to drain a pool. 
  So, I made this method public.

> Oops, clear_pool() needs to lock the mutex of course.

Done.