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

[asfimport]

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

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

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.

---

Migrated from LUCENE-6113 by ryan rawson

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I agree this is a hassle, but I think it'd be dangerous to accept null in release and ignore it? It could mask a real bug (reader ref count leak) in the application.

We could simple drop the assert: the app will see NPE which should be self explanatory.

[asfimport]

ryan rawson (migrated from JIRA)

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?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

OK I think I agree; such a code case that would fall into this trap is ... poor code anyway, and we shouldn't design our APIs on the abusers.

I wish we could somehow use AutoCloseable here so you could just use try-with-resources.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Why do you call manager.acquire() inside the try? I usually do it like that:

IndexSearcher searcher = manager.acquire();
try {
  // do some work
} finally {
  manager.release(searcher);
}

The jdocs of SearcherManager also suggest to do the same thing.

if you're calling it inside the try, you should definitely check that it's not null before attempting to pass it to SM. That's the problem of calling it inside the try. If you call it outside and it fails, there's nothing to release.