commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From robert burrell donkin <robertburrelldon...@blueyonder.co.uk>
Subject Re: [pool] roadmap
Date Mon, 14 Nov 2005 23:30:43 GMT
On Fri, 2005-11-11 at 02:32 -0500, Sandy McArthur wrote:
> On 11/10/05, robert burrell donkin <robertburrelldonkin@blueyonder.co.uk> wrote:
> > thanks to sandy's hard work, looks like pool's starting to gain some
> > momentum. this seems like a good time to start a thread about the
> > future.
> 
> Here is my Pool wish list. Since these could change the behavior of
> existing code (but not source code compatability) they should probably
> be held off until Pool 2.0:

it should be not so much a case of holding off as making sure they go
into trunk (rather than the release branch). 

> * Clarify contracts described in the javadocs, specifically:

it's important that any changes are highlighted in the documentation

> * I think an [Keyed]ObjectPool.borrowObject should only return an
> object that is: 1. newly created via PoolableObjectFactory.makeObject
> or 2. previously idle and has passed through
> PoolableObjectFactory.activateObject and
> PoolableObjectFactory.validatedObject or 3. throw an exception.

sounds reasonable but can anyone think of any missing alternative use
cases?  

> * The javadacs for [Keyed]ObjectPool.borrowObject should state by
> contract that borrowed objects must be returned to the pool via
> returnObject OR invalidateObject. Currently it only lists
> returnObject.

+1

any objections?

> * The javadacs for [Keyed]ObjectPool.borrowObject should list
> NoSuchElementException as one of the thrown exceptions. Techically a
> NSEE is included with "throws Exception" but listing NSEE would help
> clients better deal with exceptions. If you are using a pool that
> limits the number of active objects then a NSEE isn't that big of a
> deal relative to any other exception which would indicate a truely
> unexpected problem. Since NoSuchElementException extends
> RuntimeException adding this wouldn't break API compatability.

+1

IMHO throwing exception is reasonable but really the API needs to be
helping the client code understand the exceptions thrown. (personally
speaking i prefer checked exceptions but they come with their own
issues.)

> * [Keyed]PoolableObjectFactory in the package description asserts that
> activateObject will be invoked on every object returned from the pool.
> The method description asserts that activeObject will be used to
> "reinitialize an instance to be returned by the pool." What is
> ambiguous is if activateObject is used to new created objects from
> makeObject.
> 
> It's my opinion that activateObject should only be used on existing
> objects that are being returned from the internal idle object pool.
> This behavior has the advantage that it's possibly more efficient. In
> my experience newly made objects are almost always in the same state
> that an activated object would be in.

+1

IMHO there will be some use cases where calling activateObject on the
constructed object is more elegant. however, there seems little point in
demanding this behaviour generally. a decorating factory calling
activateObject on the product would allow any 1.x factories which
require this to be adapted. 

> Also, It's my opinion if activateObject throws an exception than that
> idle object should be discarded and another should be borrowed from
> the pool. If this fails until there are no more idle objects and newly
> made object is needed it is much easier to write code that only lets
> exceptions from makeObject propagate out ObjectPool.borrowObject.

+1

of course Error's should be propagated. 

> * [Keyed]PoolableObjectFactory in the package description asserts that
> validateObject will only be invoked on an "activated" object. The
> method description says that "Ensures that the instance is safe to be
> returned by the pool. Returns false if this object should be
> destroyed." I pretty much agree with both of those statements.
> 
> It's my opinion that validateObject should only be invoked on: 1.
> previously idle objects that have been activated and are about to be
> borrowed from the pool or 2. optionally by returnObject before
> passivateObject is called or 3. by the idle object evictor but not
> before the evictor has called activateObject and just before the
> evictor then calls passivateObject or destroyObject.

+1

> * It's not clearly stated what is supposed to happen to other methods
> after close has been called on a pool. The existing code throws an
> IllegalStateException when any other method on the pool is invoked.
> Personally, I think only borrowObject should throw an
> IllegalStateException and other methods such as returnObject or
> invalidateObject should just silently fail. My reasoning for this is
> say you have a threaded app with a number of currently borrowed
> objects. You shut the app down which calls the close method. As it is
> now, any worker threads that try to return borrowed objects will
> encounter an IllegalStateException. I think less harm would be done if
> those methods failed silently.

+1

the expected behaviour on close is definitely important for users of
pool and is missing from the javadocs. it needs to be added. 

(the only use for those IllegalStateException's would be as an indirect
way to work out whether the pool is closed but that's a hack and
shouldn't be actively supported.

> * The [Keyed]PoolableObjectFactory getNumIdle and getNumActive methods
> currently throw an UnsupportedOperationException if they aren't
> supported. I kinda think it would be easier on clients if they simply
> reuturned a negative value when those operations are not supported. As
> it is now if you have code similar to:
> if (pool.getNumActive() > 15) { log.message("borrowed more than expected"); }
> you really need to wrap it in a try/catch to be safe which I find a
> bit silly. Since UnsupportedOperationException is a RuntimeException
> removing this doesn't break API compatability.

this is a question of style and i've heard both sides of this argument.
however, i do suspect that the exception handling for pool is rather an
arcane art and over complex. there may be an argument for allowing
implementations flexibility as to whether they thrown an exception or
return a negative value. 

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message