lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael McCandless (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (LUCENE-3439) add checks/asserts if you search across a closed reader
Date Fri, 16 Sep 2011 19:26:08 GMT

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

Michael McCandless commented on LUCENE-3439:
--------------------------------------------


bq. 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.

bq. 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.

bq. 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.


> add checks/asserts if you search across a closed reader
> -------------------------------------------------------
>
>                 Key: LUCENE-3439
>                 URL: https://issues.apache.org/jira/browse/LUCENE-3439
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Robert Muir
>         Attachments: LUCENE-3439_test.patch
>
>
> if you try to search across a closed reader (and/or searcher too),
> there are no checks, not even assertions statements.
> this results in crazy scary stacktraces deep inside places like FSTs/various term dictionary
implementations etc.
> In some situations, depending on codec, you wont even get an error (i'm sure its fun
when you try to retrieve the stored fields!)

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

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


Mime
View raw message