[jira] [Comment Edited] (POOL-411) NPE when deregistering key at end of borrow

["Phil Steitz (Jira)" <ji...@apache.org>]

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

Phil Steitz edited comment on POOL-411 at 7/7/23 3:50 PM:
----------------------------------------------------------

[~sebb] - thanks for looking at this.  You are right - the scenario I posted (and deleted) is impossible.  The jdk docs are a little vague, but I have confirmed you are right - the thread trying to get the write lock will have to block until any readers release their read locks.  The null guard that was added in deregister prevents the exception reported above from happening, but the test case that Gary added now fails sporadically (very rarely) with this in the stack trace:

{{java.lang.NullPointerException: Cannot invoke "org.apache.commons.pool2.impl.GenericKeyedObjectPool$ObjectDeque.getIdleObjects()" because the return value of "java.util.Map.get(Object)" is null}}
{{    at org.apache.commons.pool2.impl.GenericKeyedObjectPool.addIdleObject(GenericKeyedObjectPool.java:308)}}
{{    at org.apache.commons.pool2.impl.GenericKeyedObjectPool.addObject(GenericKeyedObjectPool.java:333)}}
{{    at org.apache.com}}

The problem with above is that addObject calls register on the key before invoking addIdleObject so any deregister that happens in between should not be able to delete the pool.

The intent is for register to signal that a thread is "interested" in a key so others should refrain from deleting it.  It will also create an empty pool if there is no pool under the given key. It returns the pool under the registered key.  Calling deregister says the thread is no longer interested and also checks to see if the associated keyed pool is empty, has no objects under management and has no other threads interested in it.  In that case, it deletes it.  This setup ensures that a) when you call register, you can be sure that you will get a non-null pool and that pool will not be deleted until after you deregister it b) empty pools with no objects under management and no interested threads get cleaned up. Thread interest is tracked in the numInterested counter attached to the pool itself, so what must be going on is that counter must be getting corrupted somehow.  One way that could happen is if deregister is called twice for one registration.{{{{}}{}}}


was (Author: psteitz):
[~sebb] - thanks for looking at this.  You are right - the scenario I posted (and deleted) is impossible.  The jdk docs are a little vague, but I have confirmed you are right - the thread trying to get the write lock will have to block until any readers release their read locks.  The null guard that was added in deregister prevents the exception reported above from happening, but the test case that Gary added to now fails sporadically (very rarely) with this in the stack trace:

{{java.lang.NullPointerException: Cannot invoke "org.apache.commons.pool2.impl.GenericKeyedObjectPool$ObjectDeque.getIdleObjects()" because the return value of "java.util.Map.get(Object)" is null}}
{{    at org.apache.commons.pool2.impl.GenericKeyedObjectPool.addIdleObject(GenericKeyedObjectPool.java:308)}}
{{    at org.apache.commons.pool2.impl.GenericKeyedObjectPool.addObject(GenericKeyedObjectPool.java:333)}}
{{    at org.apache.com}}

The problem with above is that addObject calls register on the key before invoking addIdleObject so any deregister that happens in between should not be able to delete the pool.

The intent is for register to signal that a thread is "interested" in a key so others should refrain from deleting it.  It will also create an empty pool if there is no pool under the given key. It returns the pool under the registered key.  Calling deregister says the thread is no longer interested and also checks to see if the associated keyed pool is empty, has no objects under management and has no other threads interested in it.  In that case, it deletes it.  This setup ensures that a) when you call register, you can be sure that you will get a non-null pool and that pool will not be deleted until after you deregister it b) empty pools with no objects under management and no interested threads get cleaned up. Thread interest is tracked in the numInterested counter attached to the pool itself, so what must be going on is that counter must be getting corrupted somehow.  One way that could happen is if deregister is called twice for one registration.{{{}{}}}

> NPE when deregistering key at end of borrow
> -------------------------------------------
>
>                 Key: POOL-411
>                 URL: https://issues.apache.org/jira/browse/POOL-411
>             Project: Commons Pool
>          Issue Type: Task
>    Affects Versions: 2.11.1
>            Reporter: Richard Eckart de Castilho
>            Priority: Major
>             Fix For: 2.12.0
>
>
> There is a potential for an NPE happening in the finally block of borrowObject:
> {noformat}
> Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.commons.pool2.impl.GenericKeyedObjectPool$ObjectDeque.getNumInterested()" because "objectDeque" is null
> 	at org.apache.commons.pool2.impl.GenericKeyedObjectPool.deregister(GenericKeyedObjectPool.java:821)
> 	at org.apache.commons.pool2.impl.GenericKeyedObjectPool.borrowObject(GenericKeyedObjectPool.java:507)
> 	at org.apache.commons.pool2.impl.GenericKeyedObjectPool.borrowObject(GenericKeyedObjectPool.java:350) 
> {noformat}
> From reading the code, it seems this could happen e.g. if a pool is concurrently cleared while a borrow is in progress.
> Not sure what a proper solution here would be. Maybe deregister should silently do nothing if poolMap.get(k) returns null? 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)