and we shouldn't penalize correct programs
I agree we should keep the added cost for correct programs to a
minimum, but there are many good reasons why we should try to throw
AlreadyClosedException instead of something scary like NPE or
EOFException, etc.:
- Better usability – when you make a simple mistake (IW/IR.close in
the wrong place) you get a clear indication what's wrong, and that
saves you time/frustration/hair/iterations.
- Earlier error detection: lower risk that an app will push to
production without catching that IW/IR.close is wrong.
- Save Lucene devs time: when users come to the list w/ a cryptic
exception that after a few back & forths we discover was an errant
IW/IR.close, that uses up lots of people's time, time that instead
could be spent towards improving Lucene/Solr instead.
- Protect Lucene's perceived quality: like it or not, Lucene (not
the app) will sometimes be blamed when a user hits a cryptic
exception because of an errant IW/IR.close. Blog posts will go
up, tweets will get tweeted, emails to list or to other lists will
go unanswered, people doing future Googling will hit these and
see Lucene as buggy.
- We can find our own bugs – in adding some missing ensureOpens to
IW, here, I discovered a case where DirectoryReader.close calls
IW.deleteUnusedFiles after IW was closed, and this invokes the
DelPolicy, which is definitely dangerous.
Net/net I think it's important that we try, when possible/reasonable,
to have clear error detection and reporting, making it as easy as we
can for the user to understand what's wrong. And if that means a
miniscule extra cost to "correct" users, that's a fair tradeoff.
While a volatile read on (current) x86 is a no-op, it still imposes a happens-before/after restriction via the memory model and hence prevents optimizations across that volatile read. Volatile reads may not always be a no-op on x86 either (and they already aren't on other CPUs).
The thing is, we already call ensureOpen only in places where the
added cost should be vanishingly small compared to what will follow;
it'll still be vanishingly small as a volatile.
Actually, IR's ensureOpen checks the refCount (AtomicLong)
so we are already "effectively" volatile for IR, just not for IW.
Using a volatile also doesn't buy us much either - it's still best-effort (the close could come after we have checked the volatile, but before we have done the read).
Right this will always be a best effort check, but if we can do a
"better" best effort, while still having no real added cost, I think
we should improve.
I'm not saying we should add ensureOpen() in hotspots. But I do think
we should make IW's closed boolean volatile. The added cost will
still be tiny, since we only call ensureOpen where it shouldn't
matter, and it may allow us to throw ACE instead of a cryptic
exception in some cases.
I think we should add real checks here, w/ volatile isOpen/isClosed bool, up and down the stack, to those methods where the volatile read cost will be tiny compared to the cost of the method or of the app using the returned result.
IR/IW already try to do this (hmm: though not volatile for IW) but it sounds like we are missing checks inside the codecs?