don't use finalizers for FSIndexInput clones

• Key: LUCENE-750
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Yonik Seeley
• Reporter: Yonik Seeley
• Component/s: Search
• Affects Version/s: 2.1

Description

finalizers are expensive, and we should avoid using them where possible.
It looks like this helped to tickle some kind of bug (looks like a JVM bug?)
http://www.nabble.com/15-minute-hang-in-IndexInput.clone%28%29-involving-finalizers-tf2826906.html#a7891015

Comments

Changes:

• moved finalizer to the resource that actually needs it... the Descriptor (subclass of RandomAccessFile). The Descriptor does not get cloned, so we eliminate the extra finalizer per IndexInput clone.
• Descriptor is now the class that keeps track of if it has been closed or not.
• made Descriptor static... it didn't need to be a normal inner class.

[Yonik Seeley]

Forgot to remove the finalizer from FSIndexInput in the first patch.
Note that I also removed the finalizer from FSIndexOutput because IndexWriter already has a finalizer, and finalization for writers doesn't really make much sense.

Can anyone think of a reason we should keep the FSIndexOutput finalizer?

[Michael McCandless]

For FSIndexOutput ... assuming we are quite certain that nowhere in
Lucene do we fail to close an IndexOutput using a try/finally (oh, I
see one, due to lockless commits: I will fix that case), then I think
finalize() in FSIndexOutput buys us absolutely nothing? Because, all
uses of FSIndexOutput are "transient" (open, write stuff now, close).

For FSIndexInput ... assuming we are also quite certain that we close
all IndexInputs we had opened for transient purposes (eg while merging
segments, reading fields/norms, etc.) then the only thing finalize()
buys for us is protection from user failing to close an IndexReader,
due either to a bug in their code or I guess handling Exception cases,
right?

Honestly it's even tempting to not keep this finalize (due risk of
degraded GC perf, JVM bugs, etc.) Though, hmmm, I guess removing this
finalize could be seen as a regression if indeed people are relying on
GC to close their readers. So I guess we should keep this one (like
you have).

[Yonik Seeley]

committed.