commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lee Breisacher <LBreisac...@seagullsoftware.com>
Subject [pool] addObject/makeObject synchronization again
Date Mon, 07 Jul 2008 16:46:12 GMT
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:
http://issues.apache.org/jira/browse/POOL-108?focusedCommentId=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: user-unsubscribe@commons.apache.org
For additional commands, e-mail: user-help@commons.apache.org


Mime
View raw message