[jira] Created: (POOL-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

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

[jira] Created: (POOL-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

JIRA jira@apache.org
References to GenericKeyedObjectPool._minIdle are not always synchronized
-------------------------------------------------------------------------

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


References to GenericKeyedObjectPool._minIdle are not always synchronized:

The ensureMinIdle() and evict() methods both access the field outside the synch. block.
This is particularly bad as they are called from the timer thread.

The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.

Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.


--
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-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

JIRA jira@apache.org

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

Sebb commented on POOL-158:
---------------------------

Note that GenericObjectPool is OK in this regard because it only accesses _minIdle via the synch. getter/setter after construction.

> References to GenericKeyedObjectPool._minIdle are not always synchronized
> -------------------------------------------------------------------------
>
>                 Key: POOL-158
>                 URL: https://issues.apache.org/jira/browse/POOL-158
>             Project: Commons Pool
>          Issue Type: Bug
>            Reporter: Sebb
>
> References to GenericKeyedObjectPool._minIdle are not always synchronized:
> The ensureMinIdle() and evict() methods both access the field outside the synch. block.
> This is particularly bad as they are called from the timer thread.
> The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.
> Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.

--
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-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

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

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

Phil Steitz updated POOL-158:
-----------------------------

        Fix Version/s: 1.5.5
    Affects Version/s: 1.5.4
                       1.5.3
                       1.5.2
                       1.5.1
                       1.5
                       1.4
                       1.2

For correctness in evict, we probably technically need to move the read inside the synch block (otherwise in theory a write between lock acquisitions could invalidate the decision).  The ensureMinIdle read is similar, so moving both reads inside the synchs (carefully :) seems best.  Big +1 for deprecating the setter with aim to make final in 2.0.

> References to GenericKeyedObjectPool._minIdle are not always synchronized
> -------------------------------------------------------------------------
>
>                 Key: POOL-158
>                 URL: https://issues.apache.org/jira/browse/POOL-158
>             Project: Commons Pool
>          Issue Type: Bug
>    Affects Versions: 1.2, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> References to GenericKeyedObjectPool._minIdle are not always synchronized:
> The ensureMinIdle() and evict() methods both access the field outside the synch. block.
> This is particularly bad as they are called from the timer thread.
> The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.
> Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.

--
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] Resolved: (POOL-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

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

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

Phil Steitz resolved POOL-158.
------------------------------

    Resolution: Fixed

After looking carefully at the code, I concluded that making the field volatile is a better solution.  For the evict() read, there is no other reason to acquire a pool lock in the initial check.

> References to GenericKeyedObjectPool._minIdle are not always synchronized
> -------------------------------------------------------------------------
>
>                 Key: POOL-158
>                 URL: https://issues.apache.org/jira/browse/POOL-158
>             Project: Commons Pool
>          Issue Type: Bug
>    Affects Versions: 1.2, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> References to GenericKeyedObjectPool._minIdle are not always synchronized:
> The ensureMinIdle() and evict() methods both access the field outside the synch. block.
> This is particularly bad as they are called from the timer thread.
> The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.
> Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.

--
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-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

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

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

Sebb commented on POOL-158:
---------------------------

The access methods setMinIdle/getMinIdle are still synchronized. There's no need now that _minIdle is volatile.

> References to GenericKeyedObjectPool._minIdle are not always synchronized
> -------------------------------------------------------------------------
>
>                 Key: POOL-158
>                 URL: https://issues.apache.org/jira/browse/POOL-158
>             Project: Commons Pool
>          Issue Type: Bug
>    Affects Versions: 1.2, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> References to GenericKeyedObjectPool._minIdle are not always synchronized:
> The ensureMinIdle() and evict() methods both access the field outside the synch. block.
> This is particularly bad as they are called from the timer thread.
> The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.
> Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.

--
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-158) References to GenericKeyedObjectPool._minIdle are not always synchronized

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

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

Sebb commented on POOL-158:
---------------------------

I've removed the synch. from those methods

> References to GenericKeyedObjectPool._minIdle are not always synchronized
> -------------------------------------------------------------------------
>
>                 Key: POOL-158
>                 URL: https://issues.apache.org/jira/browse/POOL-158
>             Project: Commons Pool
>          Issue Type: Bug
>    Affects Versions: 1.2, 1.4, 1.5, 1.5.1, 1.5.2, 1.5.3, 1.5.4
>            Reporter: Sebb
>             Fix For: 1.5.5
>
>
> References to GenericKeyedObjectPool._minIdle are not always synchronized:
> The ensureMinIdle() and evict() methods both access the field outside the synch. block.
> This is particularly bad as they are called from the timer thread.
> The references could use getMinIdle(), which is synch., or it would probably be cheaper to move the references into the synch. blocks.
> Better yet, can the field be made final? setMinIdle() is currently only called from Test cases and setConfig() which is not referenced AFAICT.

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