StackObjectPool.borrowObject infinate loop when makeObject returns null

• Key: POOL-91
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Sandy McArthur
• Reporter: Sandy McArthur
• Fix Version/s: 2.0

Description

StackObjectPool.borrowObject has a infinate loop when makeObject returns null.

Comments

[Henri Yandell]

Is there any reason for null to be a legal makeObject return? ie) could we throw a RuntimeException of some kind when null is returned?

[Sandy McArthur]

makeObject returning null is definitely bogus behavior, but in older version of Pool (1.2 and before) StackObjectPool would return null if makeObject returned null. The question is: do we want to go with older behavior or do we want to go with what is less backwards compatible behavior but IMO better?

[Henri Yandell]

What would be bad about returning null again?

[Sorry for the dumb question - I've not done much Pool dev in the past so new to a lot of it]

[Sandy McArthur]

For me it's an issue of usability. I view a pool as a source of reusable heavyweight objects. If the pool is unable to provide one, then it's a problem and thus an Exception. If the pool returns null then you have two ways of expressing a problem and now the client code has to deal with two ways of identifying a problem.:

try {
SomeObject o = pool.borrowObject();
// do work
} catch (Exception e) {
// abort, the pool couldn't provide an instance
}

versus:

try {
SomeObject o = pool.borrowObject();
if (o == null) { // abort, the pool couldn't provide an instance }
// do work
} catch (Exception e) {
// also abort, the pool couldn't provide an instance
}

What initially got me involved with Pool was that I was duplicating code both in the PoolableObjectFactory and after check the result of pool.borrowObject() because the pool either didn't do all it could to succeed and only throw an exception when there was a real problem. The Pool roadmap is a reflection of my frustrations: http://wiki.apache.org/jakarta-commons/PoolRoadMap

[Ben Speakmon]

I'm not sure I like throwing the exception either. I can think of situations where I might want borrowings to fail silently or even have the borrower create its own instance of the object if it can't get one from the pool.

This is admittedly a major change, but I think it would be neat to take exceptions out of the borrowObject() signature and allow users to specify a handler for when borrow() fails. Imagine an interface like:

public interface FailedBorrowHandler() {
void failedBorrow(ObjectPool pool);
}

and a method in ObjectPool:

void setFailedBorrowHandler(FailedBorrowHandler handler);

Clients can then implement this interface to do whatever they want when a borrow fails. Maybe the objects in the pool aren't expected to be readily available and so borrowObject() is expected to fail half the time; in that case, requiring the caller to handle a checked exception is a pain, not a gain. If you're pooling threads, you may want to wait a few seconds and then try to borrow again, or even kill a currently engaged low-priority thread and replace it with your high-priority task.

As I said, this is a huge change, but after having used this approach in JDK 1.5's java.util.concurrent.ThreadPoolExecutor, I've become a big fan of handling pools like this.

[Ben Speakmon]

Also, I'd be happy to come up with some sample policies that implement such an interface. I'll put together an example that does it for one of the pools.

[Ben Speakmon]

As promised, a quick thrown-together example of what the API would be like with the policy I've suggested.

[Holger Hoffstätte]

Hi - I've been following this thread for a while and understand the pros and cons of both the lenient and strict approaches for borrowObject, though I'm undecded about the null vs. exception behaviour. Anyway, my question is: wouldn't this be better handled by a decorated factory instead of pluggable strategies, which will only make things more complicated? After all this behaviour is very much a one-time/configuration thing.

[Sandy McArthur]

I think there is a fundamental difference between a ThreadPoolExecutor and ObjectPool. Mainly that with a Executor your client code hands it a Runnable to execute and forget about it but with a Pool your client code keeps interacting with the pool many times. Also the Executor has it's own Threads and is asynchronous to the client code; you never know when the submitted Runnable may throw an exception. Pools are synchronous and you know that borrowObject is not going to fail except for when it's called by your client code. My point is the only way to be notified or deal with errors for an Executor is via callbacks (or sub-classing) but since there is direct interaction with the pool, error handling callbacks aren't needed.

[Ben Speakmon]

I'm interested in the decorated factory approach – how would you see that API working?

Sandy, your point about the concurrent library being fundamentally different in approach from commons-pool is well taken; perhaps it would be helpful if I explained how I've seen pool. I thought of pool as a general-purpose pool including support for threads as well as POJOs, and under that assumption the ThreadPoolExecutor approach made good sense. Of course when you're pooling heavyweight objects that don't implement Runnable, the thread-based approach doesn't really apply, and the callback is overkill.

If pool isn't about threads, and the answer to someone who wants thread pooling is to either use JDK 5 or util.concurrent, then I'm perfectly happy with that. However, on a purely API basis, I'd still like to entertain the idea of not forcing the client to catch a checked exception when borrowing an object. A factory approach could lead that way, and I'd be interested to see where that leads.

[Sandy McArthur]

Ben, a pool can pool any "Poolable Object". That could be a Thread or just about any other reusable object. There is a difference between a Pool of pooled Threads and Threads that are part of the Pool but not the pooled objects themselves. The PoolableObjectFactory is sufficient to manage the former but Executor needs a policy because it is the latter.

I'm all for moving away from forcing the client code to deal with exceptions but sometimes exceptions make sense. The Pool road map explains what I'm trying to do about that: http://wiki.apache.org/jakarta-commons/PoolRoadMap

[Sandy McArthur]

Committed a fix to throw a NoSuchElementException when makeObject returns a null.