[jira] [Commented] (LUCENE-6113) ReferenceManager.release uses assertion to expect argument not null, also expects argument to be not null

2014-12-16 Thread ryan rawson (JIRA)

[ 
https://issues.apache.org/jira/browse/LUCENE-6113?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14248848#comment-14248848
 ] 

ryan rawson commented on LUCENE-6113:
-

most people should be seeing the NPE because assertions are not always enabled.

The bigger question to me is API design. as it stands, people MUST guard their 
call in to this method call since there is no way to ensure that it will always 
be not-null (since the place you get one may fail).

As an illustrative example, in the Cocoa API, the 'release' refcount calls will 
do nothing to a null reference for similar reasons (dont spread null checking 
everywhere else in the code).

So finally, what's the code scenario whereby a 'reader ref count leak' could be 
made by doing the ignore a null release?  The app code already has to make that 
if guard, so why not absorb it in to the API?

 ReferenceManager.release uses assertion to expect argument not null, also 
 expects argument to be not null
 -

 Key: LUCENE-6113
 URL: https://issues.apache.org/jira/browse/LUCENE-6113
 Project: Lucene - Core
  Issue Type: Bug
Affects Versions: 4.10.1
Reporter: ryan rawson

 A common use pattern for the Reference Manager looks like so:
 {code}
 IndexSearcher searcher = null;
 try {
 searcher = searcherManager.acquire();
 // do real work
 } finally {
searcherManager.release(searcher);
 }
 {code}
 The problem with this code is if 'acquire' throws an exception, the finally 
 block is called with a null reference for 'searcher'.  There are two issues, 
 one is this call release() uses assertion to check for argument validity, 
 which is not recommended 
 (http://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html) 
 and secondly to fix this, we need to guard all calls to release with an if 
 clause.
 Why not have release() be a noop if it is passed null, instead of triggering 
 an NPE?  It would support this API usage pattern w/o any changes on the 
 behalf of users.
 Looking at the code, it appears that it is very unlikely that the acquire() 
 call throws an exception. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org



[jira] [Created] (LUCENE-6113) ReferenceManager.release uses assertion to expect argument not null, also expects argument to be not null

2014-12-15 Thread ryan rawson (JIRA)
ryan rawson created LUCENE-6113:
---

 Summary: ReferenceManager.release uses assertion to expect 
argument not null, also expects argument to be not null
 Key: LUCENE-6113
 URL: https://issues.apache.org/jira/browse/LUCENE-6113
 Project: Lucene - Core
  Issue Type: Bug
Affects Versions: 4.10.1
Reporter: ryan rawson


A common use pattern for the Reference Manager looks like so:

{code}
IndexSearcher searcher = null;
try {
searcher = searcherManager.acquire();
// do real work
} finally {
   searcherManager.release(searcher);
}
{code}

The problem with this code is if 'acquire' throws an exception, the finally 
block is called with a null reference for 'searcher'.  There are two issues, 
one is this call release() uses assertion to check for argument validity, which 
is not recommended 
(http://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html) 
and secondly to fix this, we need to guard all calls to release with an if 
clause.

Why not have release() be a noop if it is passed null, instead of triggering an 
NPE?  It would support this API usage pattern w/o any changes on the behalf of 
users.

Looking at the code, it appears that it is very unlikely that the acquire() 
call throws an exception. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org