Insufficient control over concurrent access to pooled objects by Evictor, client threads

• Key: POOL-125
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Phil Steitz
• Affects Version/s: 1.2, 1.4
• Fix Version/s: 1.5

Description

In pool 1.2, 1.4, borrowObject, returnObject, invalidateObject are not fully synchronized. This creates the potential for race conditions vis a vis the Evictor and/or client object lock contention. Factory methods on pooled objects should not be allowed to be invoked concurrently by client threads and the Evictor.

Comments

[John Bollinger]

I've been looking at this in the version 1.x source, and I'm not sure I see the problem. The Evicter operates by invoking the synchronized GenericObjectPool.evict() method, which touches only objects currently idle in the pool (_pool). None of borrowObject(), returnObject(), and invalidateObject() access the pool without synchronization, and none of them invoke factory methods on idle pooled objects. The only opportunities I see for factory methods to be concurrently invoked on the same object are

• if a client thread returns or invalidates an object that is already idle in the pool then that thread and the Evicter thread might concurrently invoke factory methods on it
• if two client threads attempt to return / invalidate the same object at the same time then those threads might concurrently invoke factory methods on it
• (or if the client holds references to the factory and an idle pooled object, and directly passes the object to a factory method while the Evicter is running)

Those scenarios appear to arise from client errors, and they are likely to cause problems even if the concurrency can be prevented. (For example, it may be a problem to invoke destroyObject() twice sequentially with the same argument.) What am I missing here?

Take a look at DBCP-44. There is at least one use case described in that bug that, despite correct usage by the user, can result in a deadlock.

As an aside, I have a patch for this for GOP and am in the process of porting similar changes to GKOP.

[John Bollinger]

Aha. I misunderstood the nature of the problem. I thought it was about two or more factory methods being invoked concurrently on the same pooled object, but it's really about issues that can arise in general if the client-provided object factory is not thread safe. (Right?) I think it's rather accommodating of you to handle it with a code fix. Indeed, I cannot imagine how a comprehensive fix could avoid unnecessarily reducing performance in cases where the object factory in use is already thread safe. I also don't see how Pool can protect against non-thread-safe object factories being used concurrently by the Pool and by another client (which is, in a sense, what happened in DBCP-44).

You said you already have a patch for GOP, but are you sure it wouldn't be better to document that users must provide a thread-safe object factory? They can always use one of the PoolUtil.synchronizedPoolableFactory() methods to get one if they need to do, though it would be better to use a factory that does not require synchronization for thread safety.

[Phil Steitz]

Fixed in 1.5 release.