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.
