[jira] Created: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
ObjectPool.invalidateObject(object) should throw an Exception if object is null
-------------------------------------------------------------------------------

                 Key: POOL-155
                 URL: https://issues.apache.org/jira/browse/POOL-155
             Project: Commons Pool
          Issue Type: Improvement
            Reporter: Sebb


ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.

It's easy to do this by mistake, see:

http://markmail.org/thread/ya22ihmghejbfzme

Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
[I'll do this shortly]

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12800679#action_12800679 ]

Sebb commented on POOL-155:
---------------------------

Updated Javadoc:

URL: http://svn.apache.org/viewvc?rev=899609&view=rev
Log:
POOL-155 Clarify that invalidateObject should only be called on a failed object, not a failed borrowObject.

Modified:
   commons/proper/pool/trunk/src/java/org/apache/commons/pool/ObjectPool.java

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>            Reporter: Sebb
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sebb updated POOL-155:
----------------------

    Affects Version/s: 1.5.4

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.5.4
>            Reporter: Sebb
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855568#action_12855568 ]

Phil Steitz commented on POOL-155:
----------------------------------

The issue is broader than the statement above and not really solvable without changing the pool contract.   Under the current contract, it is the cilent's responsibility to ensure that objects are not returned to the pool twice in sequence without a borrow in between and that only objects borrowed from the pool can be returned or invalidated.   To really resolve the broader issue here, the pool contract would have to be extended to include verifying that objects being returned or invalidated "belong" to the pool - i..e., have been returned by a previous activation of borrowObject.  This would require significant change to pool implementations and likely changes to user-supplied factory contracts as well.

I am +1 for additional clarification of javadoc to include clear warnings that invoking invalidateObject on objects not borrowed from the pool and other violations of the pool contract can corrupt pool counters.  I am -0 on eventual change to the contact, -1 on change before 2.0.  So I propose that we resolve by doc fixes for 1.5.5.  We can reopen and mark as 2.0 if others feel strongly that the pool contract should be extended to prevent client API abuse.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.5.4
>            Reporter: Sebb
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phil Steitz updated POOL-155:
-----------------------------

        Fix Version/s: 1.5.5
    Affects Version/s: 1.5.3
                       1.5.2
                       1.5.1
                       1.5
                       1.4
                       1.3
                       1.2
                       1.1
                       1.0.1
                       1.0

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855580#action_12855580 ]

Sebb commented on POOL-155:
---------------------------

What about the specific case of the null object?
Would throwing an Exception break the contract?

I've worked on quite a few large systems where incorrect use of pool methods caused intermittent problems at run-time.
It was usually very hard to track down the cause.
So I'm +1 on implementing checking (possibly optional) at some point.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855587#action_12855587 ]

Phil Steitz commented on POOL-155:
----------------------------------

What exception?  NPE?  IAE?  Seems odd to handle only that case and also it is possible this may break some working apps - i.e., apps that either (incorrectly) null the reference and then call invalidate or user factories that legitimately return null objects that get handed back to clients which handle validation independently of the factory methods.  In both of these cases, decrementing  numActive is correct and NPE/IAE would not be expected.

I am definitely sympathetic on the broader issue.  The tradeoff is between a simple contract and lightweight implementation (GOP does not hold references to checked out objects,  borrow/return do not have to do any equality checking and borrow/return work with direct-from-the-factory objects - i.e., the client does not have to unwrap/rewrap them) vs more robust error-prevention / instance lifecycle management by the pool itself.   We should discuss this for 2.0.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855616#action_12855616 ]

Sebb commented on POOL-155:
---------------------------

OK, I did not realise that factories could legitimately return null objects.
If that were not the case, then I think it would be OK to throw IAE for a null object - as you point out, it is incorrect to null the reference before calling invalidate(), and it's not necessary.

Maybe pools should have a configuration option to disallow null objects.
This would simplify validation.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855639#action_12855639 ]

Phil Steitz commented on POOL-155:
----------------------------------

I didn't say it was legitimate, just that there is nothing in the contract or GOP implementation that prevents it ;)

There are lots of ways for clients to prevent nulls in the factory methods, so I don't think we need to add config options specifically for this.   Note that invalidateObject will throw if destroyObject does.   An argument can be made that in that case (failed destroyObject), we should not decrement numActive (since the invariant should be numActive = objects created by the pool - objects objects idle in the pool - objects destroyed).  I think this has been discussed in the past and I would be +1 for changing this in 2.0, along with a similar change to returnObject.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855657#action_12855657 ]

Sebb commented on POOL-155:
---------------------------

I just meant that if the pool was told that factories could never return null, then it could legitimately reject null as a returned object.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12855804#action_12855804 ]

Phil Steitz commented on POOL-155:
----------------------------------

One more comment on this.  Using PoolUtils.checkedPool(...) it is possible to create pools that do instance type checking for all methods that take object instances as parameters.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (POOL-155) ObjectPool.invalidateObject(object) should throw an Exception if object is null

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/POOL-155?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Phil Steitz updated POOL-155:
-----------------------------

    Fix Version/s: 2.0
                       (was: 1.5.5)

The javadoc updates in r952050 clarify the 1.x contract.  I don't think there is anything more we should do with this until 2.0, where we can modify the pool client and factory contracts as necessary.

> ObjectPool.invalidateObject(object) should throw an Exception if object is null
> -------------------------------------------------------------------------------
>
>                 Key: POOL-155
>                 URL: https://issues.apache.org/jira/browse/POOL-155
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.0, 1.0.1, 1.1, 1.2, 1.3, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 2.0
>
>
> ObjectPool.invalidateObject(object) should throw an Exception if object is null, otherwise the numActive count can get out of synch.
> It's easy to do this by mistake, see:
> http://markmail.org/thread/ya22ihmghejbfzme
> Also, the documentation for ObjectPool needs to be updated to clarify that invalidateObject should only be called if the object failed, not the borrow.
> [I'll do this shortly]

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.