commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lee Breisacher <>
Subject RE: [pool] addObject/makeObject synchronization again
Date Mon, 07 Jul 2008 17:58:07 GMT
Hm, now I've just into something else that's quite puzzling. When I set the WhenExhaustedAction
to WHEN_EXHAUSTED_BLOCK, doing a returnObject() does not seem to unblock the borrower. Am
I completely misunderstanding how WHEN_EXHAUSTED_BLOCK is supposed to work? Here's the test:

 - in thread A, exhaust the pool by borrowing all the available objects, then call borrowObject()
once more. That waits.
 - in thread B, return one of the borrowed objects. That should unblock thread A, right? It
does not.

Is this a gaping bug - hard to believe it's never been encountered before.



> -----Original Message-----
> From: Lee Breisacher []
> Sent: Monday, July 07, 2008 9:46 AM
> To: Jakarta Commons Users List
> Subject: [pool] addObject/makeObject synchronization again
> Hi all. Referring to POOL-108, recall late last year there
> was a problem with GenericObjectPool being overly
> synchronized. In particular, I'm concerned with addObject()
> and the fact that the call to factory.makeObject() was
> synchronized and causing performance problems.
> At that time, Phil Steitz made some excellent suggestions for
> fixing it - see this JIRA entry:
> =12547740#action_12547740, and the patches attached to
> POOL-108 indeed fix the problem. Related to this is POOL-93
> which has some similar patches attached to it. However, after
> the various patches were applied, version 1.4 went out like this:
>     public synchronized void addObject() throws Exception {
>         . . . .
>         Object obj = _factory.makeObject();
>         . . . .
> Which means POOL-108 is not actually fixed! The performance
> problem is still there.
> I believe the call to makeObject should be outside of the
> lock (as seen in the patch attached to POOL-108), so  it
> should look like this:
>     public void addObject() throws Exception {
>         assertOpen();
>         if (_factory == null) {
>             throw new IllegalStateException("Cannot add
> objects without a factory.");
>         }
>         Object obj = _factory.makeObject();
>         synchronized(this) {
>         try {
>             assertOpen();
>             addObjectToPool(obj, false);
>         } catch (IllegalStateException ex) { // Pool closed
>             try {
>                 _factory.destroyObject(obj);
>             } catch (Exception ex2) {
>                 // swallow
>             }
>             throw ex;
>         }
>       }
>     }
> I pulled over the sources and made this change in my local
> copy. One of the unit tests failed, but closer examination
> revealed that the test was not quite right, so I fixed that
> and now all the unit tests pass (both with the synchronized
> change and without it).  I'm working on a new unit test right
> now that will demonstrate the problem with the current 1.4
> (overly synchronized) version.
> So, my primary question is: Does everyone agree with this
> fix? Should I re-open POOL-108, or create a new JIRA, or is
> commons-pool 1.x at end-of-life and I should just use my
> locally edited copy and be done with it?
> Note that I discussed this briefly off-list with Phil and his
> response (with further comments and questions from me) are below...
> > 1) The fixes in 1.4 solve the bulk of the real practical
> problem.  The
> > hot-and-heavy access to a production pool should not involve
> > addObject.
> > This method is only used to pre-fill a pool or by the
> > (dreaded) Evictor when trying to maintain minIdle (a bad
> practice in
> > most cases).  The basic borrow-return sequence that most
> apps hammer
> > in production has the factory methods outside of synxhronized scope
> > now.
> I don't understand why the Evictor is "dreaded" and why
> minIdle is a "bad practice". Can someone elaborate on that?
> Maintaining a minIdle works fabulously for our application.
> The objects in our pool are rather heavyweight and can take
> several seconds to create. Also, our usage pattern is such
> that a small number of objects in the pool is usually
> sufficient, but occasional spikes require extra objects, so
> having the background Evictor (obviously not a great name for
> it now that it also maintains the minIdle level) keeping the
> pool filled is a good thing.
> > 2) addObject is broken in another way that I did not want to make
> > worse in 1.4 - it does no accounting, so it is possible to add more
> > than maxActive objects to the pool using this method.
> Forcing it to
> > lock the pool while it does its work makes it less likely that the
> > pool will grow way beyond maxActive when invoking this
> method.  This
> > issue raises a larger problem that is hard to address with backward
> > compatibility in mind, which is what exactly does maxActive
> mean?  The
> > documentation is not chrystal clear and any way to make it clear
> > requires either a convoluted explanation of all of the
> strange corner
> > cases that people may be depending on in their applications or
> > cleaning things up in a way that may break some people.
> I'm not sure this problem really exists (I found the JIRA for
> it - POOL-120). Like I said, when I tweaked the unit tests
> slightly, I could no longer reproduce this problem (with the
> "weaker" synchronization like I'm suggesting).  And yeah, we
> don't use maxActive (does anyone?) and it does seem like a
> bit of an odd feature. We do have a notion of maxTotal (idle
> + active), but that's outside the pool implementation.
> Anyway, thanks for your time...
> Lee
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message