Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-818

IndexWriter should detect when it's used after being closed

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff from this thread on java-user:

      http://www.gossamer-threads.com/lists/lucene/java-user/45986

      If you call addDocument on IndexWriter after it's closed you'll hit a
      hard-to-explain NullPointerException (because the RAMDirectory was
      closed). Before 2.1, apparently you won't hit any exception and the
      IndexWrite will keep running but will have released it's write lock (I
      think).

      I plan to fix IndexWriter methods to throw an IllegalStateException if
      it has been closed.

      1. LUCENE-818.patch
        70 kB
        Michael McCandless
      2. LUCENE-818.take2.patch
        70 kB
        Michael McCandless
      3. LUCENE-818.take3.patch
        67 kB
        Michael McCandless
      4. LUCENE-818.take4.patch
        68 kB
        Michael McCandless
      5. LUCENE-818.take5.patch
        49 kB
        Michael McCandless

        Activity

        Hide
        doronc Doron Cohen added a comment -

        I looked at Java IO as a reference for acceptable behavior in this regard.

        If you close a file system object and try to use it,

        • RandomAccessFile will throw IOException with: "No such file or directory"
        • FileWriter will throw IOException with "Stream closed"
        • FileOutputStream will throw IOException with "Stream closed"

        So you could say that Java did not go all the way with a well defined illegalState exception.
        Databases on the other hand would give you a well definied error code.

        I don't have a strong opinion if this is a must for Lucene or not.

        But if this is to be added, one way to do it is to define a Closable interface

        { close(); isOpen() }

        , implement this interface in all public classes that need to check open state (IndexWriter, IndexReader, etc.), add a single static ensureOpen(Closable) method in a utils class that would throw the IllegalState exception, and just call this method from every (public?) open-state open dependent method. I think this would keep the 'noise' in the code to minimum.

        Show
        doronc Doron Cohen added a comment - I looked at Java IO as a reference for acceptable behavior in this regard. If you close a file system object and try to use it, RandomAccessFile will throw IOException with: "No such file or directory" FileWriter will throw IOException with "Stream closed" FileOutputStream will throw IOException with "Stream closed" So you could say that Java did not go all the way with a well defined illegalState exception. Databases on the other hand would give you a well definied error code. I don't have a strong opinion if this is a must for Lucene or not. But if this is to be added, one way to do it is to define a Closable interface { close(); isOpen() } , implement this interface in all public classes that need to check open state (IndexWriter, IndexReader, etc.), add a single static ensureOpen(Closable) method in a utils class that would throw the IllegalState exception, and just call this method from every (public?) open-state open dependent method. I think this would keep the 'noise' in the code to minimum.
        Hide
        mikemccand Michael McCandless added a comment -

        That approach seems a little overkill to me.

        I would need to create a new Closeable interface, and then a static
        "ensureOpen" method somewhere else. These would probably live in
        util, so then this is all public: users see that these classes
        implement Closeable, yet, it's not really a publicly useful feature,
        so it adds noise to the javadocs.

        I was planning instead on just adding a private "ensureOpen()" to
        IndexWriter (and I agree I should do IndexReader as well) that throws
        IllegalStateException if it's closed. I think noise is kept to the
        same minimum because all public methods that require this just have an
        added ensureOpen() call at the top. This is how IndexModifier works
        today.

        Hmm, IndexReader already throws IOException in certain cases if it's
        already closed. So I think I will make a new exception
        (AlreadyClosedException?) that subclasses IOException and throw that.

        Show
        mikemccand Michael McCandless added a comment - That approach seems a little overkill to me. I would need to create a new Closeable interface, and then a static "ensureOpen" method somewhere else. These would probably live in util, so then this is all public: users see that these classes implement Closeable, yet, it's not really a publicly useful feature, so it adds noise to the javadocs. I was planning instead on just adding a private "ensureOpen()" to IndexWriter (and I agree I should do IndexReader as well) that throws IllegalStateException if it's closed. I think noise is kept to the same minimum because all public methods that require this just have an added ensureOpen() call at the top. This is how IndexModifier works today. Hmm, IndexReader already throws IOException in certain cases if it's already closed. So I think I will make a new exception (AlreadyClosedException?) that subclasses IOException and throw that.
        Hide
        gsingers Grant Ingersoll added a comment -

        FWIW: Some of the other Readers (actually, IndexInput) are undefined when invoking after close. I ran into this w/ the FieldsReader when working on lazy loading. See http://www.gossamer-threads.com/lists/lucene/java-dev/34109?search_string=lazy%20field%20loading;#34109
        for reference.

        Show
        gsingers Grant Ingersoll added a comment - FWIW: Some of the other Readers (actually, IndexInput) are undefined when invoking after close. I ran into this w/ the FieldsReader when working on lazy loading. See http://www.gossamer-threads.com/lists/lucene/java-dev/34109?search_string=lazy%20field%20loading;#34109 for reference.
        Hide
        mikemccand Michael McCandless added a comment -

        I see, so IndexInput indeed does not track isClosed. It's not easy to
        fix that since IndexInput is public and non-final?

        Though I'm actually less concerned about this one because it's very
        "internal" to Lucene.

        I'm more concerned about the public paths (IndexWriter, IndexReader).
        Users seem to accidentally close these classes fairly often. I think
        IndexSearcher is OK because on close it at worst closes the underlying
        IndexReader, and then any call to that IndexReader will catch that
        it's closed.

        Oh, I see: this public path will indeed access IndexInput after it's
        been closed:

        Open reader
        Get a doc w/ lazy field(s)
        Close reader
        Try to get the lazy field's value

        That last step will clone the cloneableFieldsStream after it had been
        closed. Hmmm. Though I think we could just fix this path in
        FieldsReader by having it record when it's closed & throw an
        AlreadyClosedException if the LazyField class is used after that?
        That seems like the simplest fix.

        Show
        mikemccand Michael McCandless added a comment - I see, so IndexInput indeed does not track isClosed. It's not easy to fix that since IndexInput is public and non-final? Though I'm actually less concerned about this one because it's very "internal" to Lucene. I'm more concerned about the public paths (IndexWriter, IndexReader). Users seem to accidentally close these classes fairly often. I think IndexSearcher is OK because on close it at worst closes the underlying IndexReader, and then any call to that IndexReader will catch that it's closed. Oh, I see: this public path will indeed access IndexInput after it's been closed: Open reader Get a doc w/ lazy field(s) Close reader Try to get the lazy field's value That last step will clone the cloneableFieldsStream after it had been closed. Hmmm. Though I think we could just fix this path in FieldsReader by having it record when it's closed & throw an AlreadyClosedException if the LazyField class is used after that? That seems like the simplest fix.
        Hide
        steven_parkes Steven Parkes added a comment -

        Shades of LUCENE-686. It'd be nice to agree on that one, too (though it's internal, not external).

        Show
        steven_parkes Steven Parkes added a comment - Shades of LUCENE-686 . It'd be nice to agree on that one, too (though it's internal, not external).
        Hide
        mikemccand Michael McCandless added a comment -

        I'm planning on adding o.a.l.store.AlreadyClosedException.

        But, I'm back to subclassing this from IllegalStateException (not
        IOException). There are a fair number of methods where we should
        throw this exception but IOException was not already in the throws
        clause. Also, since it's rather rare, I think subclassing an
        unchecked Exception is appropriate.

        The only counter argument is IndexReader which currently throws
        IOException if it has been closed but then acquireWriteLock is called
        (for one of the "writer" methods). So this would technically be an
        API change for this particular case in IndexReader. But I think it's
        a tiny API change especially because it does not affect user's code
        since they already must catch the existing IOException (vs the other
        methods where adding checked "throws IOException" would be a major API
        change).

        Show
        mikemccand Michael McCandless added a comment - I'm planning on adding o.a.l.store.AlreadyClosedException. But, I'm back to subclassing this from IllegalStateException (not IOException). There are a fair number of methods where we should throw this exception but IOException was not already in the throws clause. Also, since it's rather rare, I think subclassing an unchecked Exception is appropriate. The only counter argument is IndexReader which currently throws IOException if it has been closed but then acquireWriteLock is called (for one of the "writer" methods). So this would technically be an API change for this particular case in IndexReader. But I think it's a tiny API change especially because it does not affect user's code since they already must catch the existing IOException (vs the other methods where adding checked "throws IOException" would be a major API change).
        Hide
        mikemccand Michael McCandless added a comment -

        Attached patch.

        I added checks to all public methods in IndexReader (and its
        subclasses in Lucene's sources: SegmentReader, MultiReader,
        FilteredIndexReader, ParallelReader), IndexWriter and FieldsReader
        that throw AlreadyClosedException if they are used after being closed.

        I then hit a few tests that were incorrectly closing
        reader/searcher/writer and then continuing to use them (or double
        closing them), so I fixed those.

        Show
        mikemccand Michael McCandless added a comment - Attached patch. I added checks to all public methods in IndexReader (and its subclasses in Lucene's sources: SegmentReader, MultiReader, FilteredIndexReader, ParallelReader), IndexWriter and FieldsReader that throw AlreadyClosedException if they are used after being closed. I then hit a few tests that were incorrectly closing reader/searcher/writer and then continuing to use them (or double closing them), so I fixed those.
        Hide
        doronc Doron Cohen added a comment -

        Throwing that exception for close() seems a bit too restrictive...
        Perhaps for close() of an already closed object just do nothing?
        This would be more forgiving/friendly to "over protecting" (possibly existing) applications.

        Show
        doronc Doron Cohen added a comment - Throwing that exception for close() seems a bit too restrictive... Perhaps for close() of an already closed object just do nothing? This would be more forgiving/friendly to "over protecting" (possibly existing) applications.
        Hide
        cutting Doug Cutting added a comment -

        > Perhaps for close() of an already closed object just do nothing?

        +1 Double-closing is sometimes hard to avoid. I'd rather have it ignored than force folks to, e.g., write more complicated exception handlers.

        Show
        cutting Doug Cutting added a comment - > Perhaps for close() of an already closed object just do nothing? +1 Double-closing is sometimes hard to avoid. I'd rather have it ignored than force folks to, e.g., write more complicated exception handlers.
        Hide
        mikemccand Michael McCandless added a comment -

        Excellent point. OK I fixed the patch to silently ignore close() when
        the IndexReader, IndexWriter, FieldsReader was already closed.

        Show
        mikemccand Michael McCandless added a comment - Excellent point. OK I fixed the patch to silently ignore close() when the IndexReader, IndexWriter, FieldsReader was already closed.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        I think that accessing a closed reader should continue to be undefined.
        If we define it to throw an exception, then we have thread safety issues which would be too costly (IMO) to fix.

        If we are to add checks for a closed reader, it seems like they should only be put on methods where an additional method call would be negligible. Hand-holding that decreases performance for everyone isn't something I like. Sun often goes too far in this regard, and as a consequence, people end up rewriting their own version of classes to get better performance.

        After all, this is really just making nicer error messages for incorrect programs, right? Everyone shouldn't have to pay for that. I guess I'm a minimalist

        Show
        yseeley@gmail.com Yonik Seeley added a comment - I think that accessing a closed reader should continue to be undefined. If we define it to throw an exception, then we have thread safety issues which would be too costly (IMO) to fix. If we are to add checks for a closed reader, it seems like they should only be put on methods where an additional method call would be negligible. Hand-holding that decreases performance for everyone isn't something I like. Sun often goes too far in this regard, and as a consequence, people end up rewriting their own version of classes to get better performance. After all, this is really just making nicer error messages for incorrect programs, right? Everyone shouldn't have to pay for that. I guess I'm a minimalist
        Hide
        mikemccand Michael McCandless added a comment -

        Yes, this is about giving a clear (and prompt) error message when
        Lucene is used incorrectly.

        LUCENE-140 was also a similar situation. In that case incorrect usage
        could lead to index corruption much later.

        I feel like that is important because it affects the perceived quality
        of Lucene. Enough users seem to hit this (especially common seems to
        be iterating through Hits after the reader is closed). As Lucene
        adoption grows, more users will hit it.

        Because detection of this now is so "intermittant" (undefined), a
        developer could think everything is OK in testing, push things into
        their production world, only to suddenly see an error. Then, the
        error is something strange (eg confusing NullPointerException) which
        doesn't make it clear what's happened.

        In the IndexWriter case it's potentially devastating because (I
        think?) if you keep using your closed IndexWriter, then, it's
        operating on the index without holding the write lock (this was pre
        2.1).

        I do agree we shouldn't go overboard with this (eg I certainly think
        we should not add checks inside things like TermDocs/TermPositions).

        And I agree this is in general a difficult tradeoff.

        But I think in this case the performance impact is likely very small; I could
        work out a test to be sure.

        Maybe we could remove checking for clearly frequently called methods
        (eg isDeleted)?

        Show
        mikemccand Michael McCandless added a comment - Yes, this is about giving a clear (and prompt) error message when Lucene is used incorrectly. LUCENE-140 was also a similar situation. In that case incorrect usage could lead to index corruption much later. I feel like that is important because it affects the perceived quality of Lucene. Enough users seem to hit this (especially common seems to be iterating through Hits after the reader is closed). As Lucene adoption grows, more users will hit it. Because detection of this now is so "intermittant" (undefined), a developer could think everything is OK in testing, push things into their production world, only to suddenly see an error. Then, the error is something strange (eg confusing NullPointerException) which doesn't make it clear what's happened. In the IndexWriter case it's potentially devastating because (I think?) if you keep using your closed IndexWriter, then, it's operating on the index without holding the write lock (this was pre 2.1). I do agree we shouldn't go overboard with this (eg I certainly think we should not add checks inside things like TermDocs/TermPositions). And I agree this is in general a difficult tradeoff. But I think in this case the performance impact is likely very small; I could work out a test to be sure. Maybe we could remove checking for clearly frequently called methods (eg isDeleted)?
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        > Then, the error is something strange (eg confusing NullPointerException) which doesn't make it clear what's happened.

        I think that depends on context. Many cases of NPEs are perfectly clear when you look at the stack trace.

        > In the IndexWriter case it's potentially devastating

        Agreed... when it affects correctness, we should fix.

        Adding ensureOpen() to something like maxDoc() does not ensure correctness though - an exception may or may not be thrown in the reader is already closed (because of those thread-safety issues). It actually increases the variability of behavior. We need to be careful not to guarantee the throwing of the exception.

        > especially common seems to be iterating through Hits after the reader is closed

        Good point, for document() esp. I'm OK with the heavyweight methods.

        > Maybe we could remove checking for clearly frequently called methods (eg isDeleted)?

        That's one of the ones I had in mind... isDeleted() can be called millions of times for a single query. Probably numDoc(), maxDoc(), etc, are also candidates.

        Earlier detection of bugs when the cost is nil is good though...
        what about setting more things to null when a reader is closed?

        Show
        yseeley@gmail.com Yonik Seeley added a comment - > Then, the error is something strange (eg confusing NullPointerException) which doesn't make it clear what's happened. I think that depends on context. Many cases of NPEs are perfectly clear when you look at the stack trace. > In the IndexWriter case it's potentially devastating Agreed... when it affects correctness, we should fix. Adding ensureOpen() to something like maxDoc() does not ensure correctness though - an exception may or may not be thrown in the reader is already closed (because of those thread-safety issues). It actually increases the variability of behavior. We need to be careful not to guarantee the throwing of the exception. > especially common seems to be iterating through Hits after the reader is closed Good point, for document() esp. I'm OK with the heavyweight methods. > Maybe we could remove checking for clearly frequently called methods (eg isDeleted)? That's one of the ones I had in mind... isDeleted() can be called millions of times for a single query. Probably numDoc(), maxDoc(), etc, are also candidates. Earlier detection of bugs when the cost is nil is good though... what about setting more things to null when a reader is closed?
        Hide
        mikemccand Michael McCandless added a comment -

        >> Then, the error is something strange (eg confusing NullPointerException) which doesn't make it clear what's happened.
        >
        > I think that depends on context. Many cases of NPEs are perfectly clear when you look at the stack trace.

        Well for people familiar with Lucene's sources, yes. But most of our
        users are not and so seeing an "AlreadyClosedException" can make a big
        difference over [possibly rather nested, and, rather delayed] NPE or
        something else.

        EG look at the poor user that led to my opening this issue (link in
        opening comment). The user was understandably confused by the NPE
        inside the RAMDirectory.

        > Adding ensureOpen() to something like maxDoc() does not ensure
        > correctness though - an exception may or may not be thrown in the
        > reader is already closed (because of those thread-safety issues). It
        > actually increases the variability of behavior. We need to be
        > careful not to guarantee the throwing of the exception.

        On the thread safety issue: are you saying if one thread closes the
        reader while another thread is using it, there is uncertainty excactly
        when the 2nd thread will hit the AlreadyClosedException (because of
        how the JVM schedules the threads)? I think this kind of thread
        behavior is normal/expected?

        Or is the thread safety issue something else?

        >> especially common seems to be iterating through Hits after the reader is closed
        >
        > Good point, for document() esp. I'm OK with the heavyweight methods.
        >
        >> Maybe we could remove checking for clearly frequently called methods (eg isDeleted)?
        >
        > That's one of the ones I had in mind... isDeleted() can be called millions of times for a single query. Probably numDoc(), maxDoc(), etc, are also candidates.

        OK how about we do not call ensureOpen() in these IndexReader methods?:

        numDoc()
        maxDoc()
        isDeleted()

        I think even without checking in those methods we still catch a number
        of common cases:

        • Close reader then try to run a search (termDocs()/termPositions()
          catch the close)
        • Run a search, close reader, then try to iterate through Hits
          (document() catches the close)
        • Close a reader then try to delete docs or setNorms
          (deleteDocument()/setNorm() catch the close)

        > Earlier detection of bugs when the cost is nil is good though...

        Yes I think we in general want "fail-fast" here.

        > what about setting more things to null when a reader is closed?

        Well ... I would prefer not to increase the frequency of getting
        "undefined" NPEs out of the reader. If we are going to cause
        additional exceptions here, I'd like to make them intelligible to the
        user (ie, AlreadyClosedException). EG, take SegmentReader's "si"
        (SegmentInfo). If we set this to null on close, then numDoc() and
        maxDoc() would hit NPE instead of just returning the correct answer.
        I think I'd prefer returning the correct answer for such cases.

        Show
        mikemccand Michael McCandless added a comment - >> Then, the error is something strange (eg confusing NullPointerException) which doesn't make it clear what's happened. > > I think that depends on context. Many cases of NPEs are perfectly clear when you look at the stack trace. Well for people familiar with Lucene's sources, yes. But most of our users are not and so seeing an "AlreadyClosedException" can make a big difference over [possibly rather nested, and, rather delayed] NPE or something else. EG look at the poor user that led to my opening this issue (link in opening comment). The user was understandably confused by the NPE inside the RAMDirectory. > Adding ensureOpen() to something like maxDoc() does not ensure > correctness though - an exception may or may not be thrown in the > reader is already closed (because of those thread-safety issues). It > actually increases the variability of behavior. We need to be > careful not to guarantee the throwing of the exception. On the thread safety issue: are you saying if one thread closes the reader while another thread is using it, there is uncertainty excactly when the 2nd thread will hit the AlreadyClosedException (because of how the JVM schedules the threads)? I think this kind of thread behavior is normal/expected? Or is the thread safety issue something else? >> especially common seems to be iterating through Hits after the reader is closed > > Good point, for document() esp. I'm OK with the heavyweight methods. > >> Maybe we could remove checking for clearly frequently called methods (eg isDeleted)? > > That's one of the ones I had in mind... isDeleted() can be called millions of times for a single query. Probably numDoc(), maxDoc(), etc, are also candidates. OK how about we do not call ensureOpen() in these IndexReader methods?: numDoc() maxDoc() isDeleted() I think even without checking in those methods we still catch a number of common cases: Close reader then try to run a search (termDocs()/termPositions() catch the close) Run a search, close reader, then try to iterate through Hits (document() catches the close) Close a reader then try to delete docs or setNorms (deleteDocument()/setNorm() catch the close) > Earlier detection of bugs when the cost is nil is good though... Yes I think we in general want "fail-fast" here. > what about setting more things to null when a reader is closed? Well ... I would prefer not to increase the frequency of getting "undefined" NPEs out of the reader. If we are going to cause additional exceptions here, I'd like to make them intelligible to the user (ie, AlreadyClosedException). EG, take SegmentReader's "si" (SegmentInfo). If we set this to null on close, then numDoc() and maxDoc() would hit NPE instead of just returning the correct answer. I think I'd prefer returning the correct answer for such cases.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        > On the thread safety issue: are you saying if one thread closes the
        > reader while another thread is using it, there is uncertainty excactly
        > when the 2nd thread will hit the AlreadyClosedException (because of
        > how the JVM schedules the threads)?

        Yes, but it's not just thread scheduling, it's also lack of memory barriers. The 2nd thread may never see the close(), depending on the exact architecture of machine and the JVM.

        > I think this kind of thread behavior is normal/expected?

        For a class that isn't thread safe, yes. IndexReader is advertised as being thread safe though. If we guarantee an exception accessing a closed reader, then that should work 100% of the time. I don't think we should make that guarantee.

        We can still throw meaningful errors in more cases and make it easier for the user to debug that, but it should not be deemed an error if we don't throw an exception. Users should never rely on getting this exception for flow-control purposes anyway.

        > OK how about we do not call ensureOpen() in these IndexReader methods?:
        > numDoc()
        > maxDoc()
        > isDeleted()

        +1

        hasDeletions() too?

        > > what about setting more things to null when a reader is closed?
        > Well ... I would prefer not to increase the frequency of getting "undefined" NPEs out of the reader

        Yes, but not all bugs will be user bugs. Some will be internal Lucene stuff that bypass public methods.
        It's still better that these fail quicker too. Anyway, that can be handled on a case-by-case basis later.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - > On the thread safety issue: are you saying if one thread closes the > reader while another thread is using it, there is uncertainty excactly > when the 2nd thread will hit the AlreadyClosedException (because of > how the JVM schedules the threads)? Yes, but it's not just thread scheduling, it's also lack of memory barriers. The 2nd thread may never see the close(), depending on the exact architecture of machine and the JVM. > I think this kind of thread behavior is normal/expected? For a class that isn't thread safe, yes. IndexReader is advertised as being thread safe though. If we guarantee an exception accessing a closed reader, then that should work 100% of the time. I don't think we should make that guarantee. We can still throw meaningful errors in more cases and make it easier for the user to debug that, but it should not be deemed an error if we don't throw an exception. Users should never rely on getting this exception for flow-control purposes anyway. > OK how about we do not call ensureOpen() in these IndexReader methods?: > numDoc() > maxDoc() > isDeleted() +1 hasDeletions() too? > > what about setting more things to null when a reader is closed? > Well ... I would prefer not to increase the frequency of getting "undefined" NPEs out of the reader Yes, but not all bugs will be user bugs. Some will be internal Lucene stuff that bypass public methods. It's still better that these fail quicker too. Anyway, that can be handled on a case-by-case basis later.
        Hide
        mikemccand Michael McCandless added a comment -

        >> On the thread safety issue: are you saying if one thread closes the
        >> reader while another thread is using it, there is uncertainty excactly
        >> when the 2nd thread will hit the AlreadyClosedException (because of
        >> how the JVM schedules the threads)?
        >
        > Yes, but it's not just thread scheduling, it's also lack of memory
        > barriers. The 2nd thread may never see the close(), depending on
        > the exact architecture of machine and the JVM.

        Yikes. Is this the Java memory model issue? Ie, there is no hard
        guarantee on when a "write" from one thread will be visible to other
        threads, unless you use "volatile"?

        >> I think this kind of thread behavior is normal/expected?
        >
        > For a class that isn't thread safe, yes. IndexReader is advertised
        > as being thread safe though. If we guarantee an exception accessing
        > a closed reader, then that should work 100% of the time. I don't
        > think we should make that guarantee.

        OK I think we shouldn't "guarantee" it. I think listing as "@throws
        AlreadyClosedException if this IndexReader is closed" is OK?

        > We can still throw meaningful errors in more cases and make it
        > easier for the user to debug that, but it should not be deemed an
        > error if we don't throw an exception. Users should never rely on
        > getting this exception for flow-control purposes anyway.

        Agreed.

        >> OK how about we do not call ensureOpen() in these IndexReader methods?:
        >> numDoc()
        >> maxDoc()
        >> isDeleted()
        >
        > +1
        >
        > hasDeletions() too?

        OK I will change to not call ensureOpen() for hasDeletions too. I
        will roll a new patch with this.

        >> > what about setting more things to null when a reader is closed?
        >> Well ... I would prefer not to increase the frequency of getting "undefined" NPEs out of the reader
        >
        > Yes, but not all bugs will be user bugs. Some will be internal
        > Lucene stuff that bypass public methods. It's still better that
        > these fail quicker too. Anyway, that can be handled on a
        > case-by-case basis later.

        OK, I agree. Better to throw a "fail-fast" NPE than do something
        strange later.

        Show
        mikemccand Michael McCandless added a comment - >> On the thread safety issue: are you saying if one thread closes the >> reader while another thread is using it, there is uncertainty excactly >> when the 2nd thread will hit the AlreadyClosedException (because of >> how the JVM schedules the threads)? > > Yes, but it's not just thread scheduling, it's also lack of memory > barriers. The 2nd thread may never see the close(), depending on > the exact architecture of machine and the JVM. Yikes. Is this the Java memory model issue? Ie, there is no hard guarantee on when a "write" from one thread will be visible to other threads, unless you use "volatile"? >> I think this kind of thread behavior is normal/expected? > > For a class that isn't thread safe, yes. IndexReader is advertised > as being thread safe though. If we guarantee an exception accessing > a closed reader, then that should work 100% of the time. I don't > think we should make that guarantee. OK I think we shouldn't "guarantee" it. I think listing as "@throws AlreadyClosedException if this IndexReader is closed" is OK? > We can still throw meaningful errors in more cases and make it > easier for the user to debug that, but it should not be deemed an > error if we don't throw an exception. Users should never rely on > getting this exception for flow-control purposes anyway. Agreed. >> OK how about we do not call ensureOpen() in these IndexReader methods?: >> numDoc() >> maxDoc() >> isDeleted() > > +1 > > hasDeletions() too? OK I will change to not call ensureOpen() for hasDeletions too. I will roll a new patch with this. >> > what about setting more things to null when a reader is closed? >> Well ... I would prefer not to increase the frequency of getting "undefined" NPEs out of the reader > > Yes, but not all bugs will be user bugs. Some will be internal > Lucene stuff that bypass public methods. It's still better that > these fail quicker too. Anyway, that can be handled on a > case-by-case basis later. OK, I agree. Better to throw a "fail-fast" NPE than do something strange later.
        Hide
        djd Daniel John Debrunner added a comment -

        >> Yes, but it's not just thread scheduling, it's also lack of memory
        >> barriers. The 2nd thread may never see the close(), depending on
        >> the exact architecture of machine and the JVM.

        >Yikes. Is this the Java memory model issue? Ie, there is no hard
        >guarantee on when a "write" from one thread will be visible to other
        >threads, unless you use "volatile"?

        Doesn't crossing a synchronization barrier ensure that all threads will seem the same value of a field?

        Show
        djd Daniel John Debrunner added a comment - >> Yes, but it's not just thread scheduling, it's also lack of memory >> barriers. The 2nd thread may never see the close(), depending on >> the exact architecture of machine and the JVM. >Yikes. Is this the Java memory model issue? Ie, there is no hard >guarantee on when a "write" from one thread will be visible to other >threads, unless you use "volatile"? Doesn't crossing a synchronization barrier ensure that all threads will seem the same value of a field?
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        > Doesn't crossing a synchronization barrier ensure that all threads will seem the same value of a field?

        Yes, if the changing thread crosses a write barrier and the reading thread crosses a read barrier.

        The obvious implication that you need to synchronize the sets/gets of isClosed. That's not a big deal in some cases, but in anything that could be in an inner loop, it is a big deal. We already have reports of simple things like isDeleted() being a bottleneck on servers with many CPUs and concurrent threads because of the synchronization. That particular sync point is something that I've planned to get rid of at some point in the future by having a "read only" IndexReader.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - > Doesn't crossing a synchronization barrier ensure that all threads will seem the same value of a field? Yes, if the changing thread crosses a write barrier and the reading thread crosses a read barrier. The obvious implication that you need to synchronize the sets/gets of isClosed. That's not a big deal in some cases, but in anything that could be in an inner loop, it is a big deal. We already have reports of simple things like isDeleted() being a bottleneck on servers with many CPUs and concurrent threads because of the synchronization. That particular sync point is something that I've planned to get rid of at some point in the future by having a "read only" IndexReader.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        > Yikes. Is this the Java memory model issue?

        Yes, but it's not a problem with the Java memory model, it's more of a feature. Threads in general have this problem (or perform very poorly if they don't). It's also very much about compiler optimizations as well. The optimizer may stuff a value in a register, and never check it's location in main-memory, and never see the changes. Even if it doesn't then you drop down to problems with the CPU and cache + memory architectue in SMP systems. Some systems require explicit memory barriers or cache flushes to see memory changes from another CPU. One CPU writes to a spot in memory, and absent any special instructions, another CPU reading from that spot in memory may not see the change.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - > Yikes. Is this the Java memory model issue? Yes, but it's not a problem with the Java memory model, it's more of a feature. Threads in general have this problem (or perform very poorly if they don't). It's also very much about compiler optimizations as well. The optimizer may stuff a value in a register, and never check it's location in main-memory, and never see the changes. Even if it doesn't then you drop down to problems with the CPU and cache + memory architectue in SMP systems. Some systems require explicit memory barriers or cache flushes to see memory changes from another CPU. One CPU writes to a spot in memory, and absent any special instructions, another CPU reading from that spot in memory may not see the change.
        Hide
        mikemccand Michael McCandless added a comment -

        Attached take3, which just removes ensureOpen() checks for IndexReader's maxDoc(), numDoc(), hasDeletions(), isDeleted(...).

        Show
        mikemccand Michael McCandless added a comment - Attached take3, which just removes ensureOpen() checks for IndexReader's maxDoc(), numDoc(), hasDeletions(), isDeleted(...).
        Hide
        hossman Hoss Man added a comment -

        I havne't been following this issue that closely, and i haven't read any of the patches, but if the goal is:

        "reduce situations where novice users get confusing exceptions when using IndexWriter after closing it"

        ...and the concern is that calling a new "ensureOpen()" method at the begining of every public method may be a performacne issue, then perhaps an alternative would be to catch all exceptions inside public methods and wrap them if not closed, ie...

        public Foo somePublicMethod() throws IOException {
        try

        { // current method body }

        catch (Exception e) {
        if (isOpen())

        { throw e; }

        else

        { throw new AllreadyClosedException(e); }

        }
        }

        ...that way any performance cost of ensureOpen/isOpen is only payed on an existing Exception. the down side i see to this approach is that since it only explains exceptions hte caller would get anyway, it doesn't give any feedback about using a closed writer in cases where the current code returns cleanly (but may not function properly) ... we might be able to solve that by adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions.

        Show
        hossman Hoss Man added a comment - I havne't been following this issue that closely, and i haven't read any of the patches, but if the goal is: "reduce situations where novice users get confusing exceptions when using IndexWriter after closing it" ...and the concern is that calling a new "ensureOpen()" method at the begining of every public method may be a performacne issue, then perhaps an alternative would be to catch all exceptions inside public methods and wrap them if not closed, ie... public Foo somePublicMethod() throws IOException { try { // current method body } catch (Exception e) { if (isOpen()) { throw e; } else { throw new AllreadyClosedException(e); } } } ...that way any performance cost of ensureOpen/isOpen is only payed on an existing Exception. the down side i see to this approach is that since it only explains exceptions hte caller would get anyway, it doesn't give any feedback about using a closed writer in cases where the current code returns cleanly (but may not function properly) ... we might be able to solve that by adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions.
        Hide
        mikemccand Michael McCandless added a comment -

        To make that work, I think we'd have to convert the Exception into
        either IOException or RuntimeException before throwing it (so we don't
        have to change method signature).

        But, I think this approach adds more noise to the source code. Also
        because it's "after the fact" then damage could have been done (I
        think IndexWriter before 2.1 would gleefully keep writing, but
        without holding the write lock!). It would only catch cases where an
        exception was thrown (many times there is none) whereas I prefer "fail
        fast".

        Show
        mikemccand Michael McCandless added a comment - To make that work, I think we'd have to convert the Exception into either IOException or RuntimeException before throwing it (so we don't have to change method signature). But, I think this approach adds more noise to the source code. Also because it's "after the fact" then damage could have been done (I think IndexWriter before 2.1 would gleefully keep writing, but without holding the write lock!). It would only catch cases where an exception was thrown (many times there is none) whereas I prefer "fail fast".
        Hide
        hossman Hoss Man added a comment -

        1) my example attempted to be concise ... ideally we would be explicit in our catches.

        2) the body of the catch clauses could be put into a helper method just like ensureOpen to help reduce code noise

        3) if there are situations where damage will be done by not testing that we are open before taking some action, that would fall under my "adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions." ... a lot of this could be achieved (as Yonik suggested) by nulling out more things in close so that the first attempt to do something dangerous after the close triggered a NullPointerException.

        4) "fail fast" is always good ... except when it makes the non-failure case slow ... i was merely suggesting an alternative that would achieve the same results without penalizing performance of people obeying the rules.

        as an added bonus, both methodologies could be used...

        if numDoc(), maxDoc(), isDeleted(), and hasDeletions() are the only mehtods were people are concerned about the performance impacts of calling ensureOpen() everytime, then those methods could be the ones where isOpen could be checked in any exception handling block, and all of the other mehtods could use ensureOpen as orriginal described.

        Show
        hossman Hoss Man added a comment - 1) my example attempted to be concise ... ideally we would be explicit in our catches. 2) the body of the catch clauses could be put into a helper method just like ensureOpen to help reduce code noise 3) if there are situations where damage will be done by not testing that we are open before taking some action, that would fall under my "adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions." ... a lot of this could be achieved (as Yonik suggested) by nulling out more things in close so that the first attempt to do something dangerous after the close triggered a NullPointerException. 4) "fail fast" is always good ... except when it makes the non-failure case slow ... i was merely suggesting an alternative that would achieve the same results without penalizing performance of people obeying the rules. as an added bonus, both methodologies could be used... if numDoc(), maxDoc(), isDeleted(), and hasDeletions() are the only mehtods were people are concerned about the performance impacts of calling ensureOpen() everytime, then those methods could be the ones where isOpen could be checked in any exception handling block, and all of the other mehtods could use ensureOpen as orriginal described.
        Hide
        mikemccand Michael McCandless added a comment -

        > 2) the body of the catch clauses could be put into a helper method just like ensureOpen to help reduce code noise

        True, but I think it's still more code noise to have
        try/catch/call-helper-method than ensureOpen() call? Esp. since you'd
        have to handle IOException and RuntimeException as 2 separate catch
        clauses (I think?), each of which calls a separate helper. Ie instead
        of:

        ensureOpen();
        ...do stuff...

        we would need something like:

        try

        { ...do stuff... }

        catch (IOException exc)

        { throw ensureOpenAfterIOException(exc); }

        catch (RuntimeException exc)

        { throw ensureOpenAfterRuntimeException(exc); }

        I think?

        > 3) if there are situations where damage will be done by not testing that we are open before taking some action, that would fall under my "adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions." ... a lot of this could be achieved (as Yonik suggested) by nulling out more things in close so that the first attempt to do something dangerous after the close triggered a NullPointerException.

        The thing is we may not know all such cases (yet)? I prefer taking a
        defensive approach here. I don't really like the null-out solution
        because I think getting an AlreadyClosedException is clearer to the
        user than a NPE's.

        > 4) "fail fast" is always good ... except when it makes the non-failure case slow ... i was merely suggesting an alternative that would achieve the same results without penalizing performance of people obeying the rules.
        >
        > if numDoc(), maxDoc(), isDeleted(), and hasDeletions() are the only mehtods were people are concerned about the performance impacts of calling ensureOpen() everytime, then those methods could be the ones where isOpen could be checked in any exception handling block, and all of the other mehtods could use ensureOpen as orriginal described.

        Good idea! An added bonus is that these methods do not throw
        IOException so the exception handling would just have the one
        RuntimeException catch clause above. OK I will rework patch with this
        approach and see how it looks...

        Show
        mikemccand Michael McCandless added a comment - > 2) the body of the catch clauses could be put into a helper method just like ensureOpen to help reduce code noise True, but I think it's still more code noise to have try/catch/call-helper-method than ensureOpen() call? Esp. since you'd have to handle IOException and RuntimeException as 2 separate catch clauses (I think?), each of which calls a separate helper. Ie instead of: ensureOpen(); ...do stuff... we would need something like: try { ...do stuff... } catch (IOException exc) { throw ensureOpenAfterIOException(exc); } catch (RuntimeException exc) { throw ensureOpenAfterRuntimeException(exc); } I think? > 3) if there are situations where damage will be done by not testing that we are open before taking some action, that would fall under my "adding better error checking in those specific cases (if we know of any) and throwing explicit exceptions." ... a lot of this could be achieved (as Yonik suggested) by nulling out more things in close so that the first attempt to do something dangerous after the close triggered a NullPointerException. The thing is we may not know all such cases (yet)? I prefer taking a defensive approach here. I don't really like the null-out solution because I think getting an AlreadyClosedException is clearer to the user than a NPE's. > 4) "fail fast" is always good ... except when it makes the non-failure case slow ... i was merely suggesting an alternative that would achieve the same results without penalizing performance of people obeying the rules. > > if numDoc(), maxDoc(), isDeleted(), and hasDeletions() are the only mehtods were people are concerned about the performance impacts of calling ensureOpen() everytime, then those methods could be the ones where isOpen could be checked in any exception handling block, and all of the other mehtods could use ensureOpen as orriginal described. Good idea! An added bonus is that these methods do not throw IOException so the exception handling would just have the one RuntimeException catch clause above. OK I will rework patch with this approach and see how it looks...
        Hide
        hossman Hoss Man added a comment -

        i would make sure to heavily document each and every use of isOpen/ensureOpen to make it clear why they are being used when they are from a performance concern case ... otherwise people will be very confused down the road ("why the hell isn't this being checked until the catch block?!?!?!")

        Show
        hossman Hoss Man added a comment - i would make sure to heavily document each and every use of isOpen/ensureOpen to make it clear why they are being used when they are from a performance concern case ... otherwise people will be very confused down the road ("why the hell isn't this being checked until the catch block?!?!?!")
        Hide
        mikemccand Michael McCandless added a comment -

        > i would make sure to heavily document each and every use of isOpen/ensureOpen to make it clear why they are being used when they are from a performance concern case ... otherwise people will be very confused down the road ("why the hell isn't this being checked until the catch block?!?!?!")

        OK I changed my mind.

        I don't think we should add the "check if closed after the fact"
        logic. I reviewed the 4 methods in question (maxDoc, numDocs,
        hasDeletions, isDeleted) and for all subclasses of IndexReader these
        are very trivial methods that won't throw exceptions.

        So I'm back to thinking this added complexity (and future confusion to
        people looking @ Lucene source code) isn't worthwhile and we should
        leave these methods with no checking for whether they are already
        closed.

        But I will document why ensureOpen is not being called in each of
        these methods.

        Show
        mikemccand Michael McCandless added a comment - > i would make sure to heavily document each and every use of isOpen/ensureOpen to make it clear why they are being used when they are from a performance concern case ... otherwise people will be very confused down the road ("why the hell isn't this being checked until the catch block?!?!?!") OK I changed my mind. I don't think we should add the "check if closed after the fact" logic. I reviewed the 4 methods in question (maxDoc, numDocs, hasDeletions, isDeleted) and for all subclasses of IndexReader these are very trivial methods that won't throw exceptions. So I'm back to thinking this added complexity (and future confusion to people looking @ Lucene source code) isn't worthwhile and we should leave these methods with no checking for whether they are already closed. But I will document why ensureOpen is not being called in each of these methods.
        Hide
        mikemccand Michael McCandless added a comment -

        New patch based on above discussion: just re-based to the current trunk, and, added comments on why ensureOpen is not called in the 4 methods.

        Show
        mikemccand Michael McCandless added a comment - New patch based on above discussion: just re-based to the current trunk, and, added comments on why ensureOpen is not called in the 4 methods.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        If we aren't guaranteeing an exception be thrown after a close, why are we bothering to put the exception in the method signature?

        It's already apparent to everyone that using something after it's closed isn't good, so we were just cleaning up the error messages and making it easier to debug. People shouldn't have specific application logic for this exception... it should be treated as a program error.

        IMO, listing the exception encourages people to catch it, suggests that they can depend on the exception being thrown, and also makes it more difficult to remove it from certain methods in the future.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - If we aren't guaranteeing an exception be thrown after a close, why are we bothering to put the exception in the method signature? It's already apparent to everyone that using something after it's closed isn't good, so we were just cleaning up the error messages and making it easier to debug. People shouldn't have specific application logic for this exception... it should be treated as a program error. IMO, listing the exception encourages people to catch it, suggests that they can depend on the exception being thrown, and also makes it more difficult to remove it from certain methods in the future.
        Hide
        doronc Doron Cohen added a comment -

        > People shouldn't have specific application logic for this exception... it should be treated as a program error.
        +1

        Also, another candidate for AlreadyClosedException upon using an already closed (RAM)Directory - came up in http://www.nabble.com/sharing-my-experience-for-upgrading-from-Lucene-1.9-to-Lucene-2.2-dev-tf3366948.html

        Show
        doronc Doron Cohen added a comment - > People shouldn't have specific application logic for this exception... it should be treated as a program error. +1 Also, another candidate for AlreadyClosedException upon using an already closed (RAM)Directory - came up in http://www.nabble.com/sharing-my-experience-for-upgrading-from-Lucene-1.9-to-Lucene-2.2-dev-tf3366948.html
        Hide
        mikemccand Michael McCandless added a comment -

        >> People shouldn't have specific application logic for this exception... it should be treated as a program error.
        > +1

        OK, agreed. I will remove AlreadyClosedException from all @throws
        method signatures.

        > Also, another candidate for AlreadyClosedException upon using an already closed (RAM)Directory - came up in http://www.nabble.com/sharing-my-experience-for-upgrading-from-Lucene-1.9-to-Lucene-2.2-dev-tf3366948.html

        Yeah I saw this too. Here's the relevant excerpt:

        > 3. RAMDirectory related changes
        > It took me something to find this out. Previously, after
        > ramDirectory = new RAMDirectory(file)
        > I could ramDirectory.close() to release the resources. And later, I
        > could do a check for IndexReader.indexExists(ramDirectory) to see if
        > there is an index in the directory. FSDirectory behaves this way also.
        > But with lucene 2.2, NullPointerExceptions came out. It turns out
        > when ramDirectory.close(), the instance variable fileMap is set to
        > null. And IndexReader.indexExists(ramDirectory) is reading fileMap to
        > look for indexes, causing the NPE.

        In fact the original user's list email that started this bug was also
        due to RAMDirectory setting its fileMap to null, but in that case, it
        was via IndexWriter so detecting that IndexWriter is closed would
        prevent that one.

        In this case the developer is using a RAMDirectory directly.

        I think this is an example where "nulling things out on close" leads
        to developer confusion. Previously RAMDirectory functioned fine after
        being closed(); now, it throws a hard to understand (unless you are
        familiar w/ Lucene's sources & what specifically we changed in 2.1)
        NPE.

        I think we should fix this?

        Since RAMDir's public methods are fairly hot (eg heavily used building
        single-doc RAM segment), we can use Hoss's neat approach and
        specifically catch the NPE and rethrow as an AlreadyClosedException?

        I don't mind if the "after close semantics" is "it works just like it
        did when it was open" (ie, close is a no-op). I also don't mind if
        you get a "fail-fast" quick AlreadyClosedException. But I think
        anything in between (NPE or other undefined, intermittant exceptions)
        only confuses our developers.

        Looking at FSDirectory, it continues to work fine after close with the
        one spooky exception that it may have been removed from the
        DIRECTORIES hashtable, which means if you then open the same canonical
        path again you get a different FSDirectory instance. The comment
        states "this permits synchronization on directories", but I don't see
        where in Lucene we are relying on this? Ie, what could break if a
        user keeps using a closed FSDirectory thus possibly having more than
        one FSDirectory instance for a given canonical path?

        Show
        mikemccand Michael McCandless added a comment - >> People shouldn't have specific application logic for this exception... it should be treated as a program error. > +1 OK, agreed. I will remove AlreadyClosedException from all @throws method signatures. > Also, another candidate for AlreadyClosedException upon using an already closed (RAM)Directory - came up in http://www.nabble.com/sharing-my-experience-for-upgrading-from-Lucene-1.9-to-Lucene-2.2-dev-tf3366948.html Yeah I saw this too. Here's the relevant excerpt: > 3. RAMDirectory related changes > It took me something to find this out. Previously, after > ramDirectory = new RAMDirectory(file) > I could ramDirectory.close() to release the resources. And later, I > could do a check for IndexReader.indexExists(ramDirectory) to see if > there is an index in the directory. FSDirectory behaves this way also. > But with lucene 2.2, NullPointerExceptions came out. It turns out > when ramDirectory.close(), the instance variable fileMap is set to > null. And IndexReader.indexExists(ramDirectory) is reading fileMap to > look for indexes, causing the NPE. In fact the original user's list email that started this bug was also due to RAMDirectory setting its fileMap to null, but in that case, it was via IndexWriter so detecting that IndexWriter is closed would prevent that one. In this case the developer is using a RAMDirectory directly. I think this is an example where "nulling things out on close" leads to developer confusion. Previously RAMDirectory functioned fine after being closed(); now, it throws a hard to understand (unless you are familiar w/ Lucene's sources & what specifically we changed in 2.1) NPE. I think we should fix this? Since RAMDir's public methods are fairly hot (eg heavily used building single-doc RAM segment), we can use Hoss's neat approach and specifically catch the NPE and rethrow as an AlreadyClosedException? I don't mind if the "after close semantics" is "it works just like it did when it was open" (ie, close is a no-op). I also don't mind if you get a "fail-fast" quick AlreadyClosedException. But I think anything in between (NPE or other undefined, intermittant exceptions) only confuses our developers. Looking at FSDirectory, it continues to work fine after close with the one spooky exception that it may have been removed from the DIRECTORIES hashtable, which means if you then open the same canonical path again you get a different FSDirectory instance. The comment states "this permits synchronization on directories", but I don't see where in Lucene we are relying on this? Ie, what could break if a user keeps using a closed FSDirectory thus possibly having more than one FSDirectory instance for a given canonical path?
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        > we can use Hoss's neat approach and
        > specifically catch the NPE and rethrow as an AlreadyClosedException?

        I'm not enough of an expert to know if this would be faster or slower than a simple fileMap==null check.
        I would guess it depends on what needs to be set up in the stack frame to potentially catch an exception, and if the try/catch/throw code prevents any optimizations (such as inlining).

        Also, catching a NPE seems a little icky to me... so w/o more info I'd lean toward a fileMap==null check (if anything).

        Show
        yseeley@gmail.com Yonik Seeley added a comment - > we can use Hoss's neat approach and > specifically catch the NPE and rethrow as an AlreadyClosedException? I'm not enough of an expert to know if this would be faster or slower than a simple fileMap==null check. I would guess it depends on what needs to be set up in the stack frame to potentially catch an exception, and if the try/catch/throw code prevents any optimizations (such as inlining). Also, catching a NPE seems a little icky to me... so w/o more info I'd lean toward a fileMap==null check (if anything).
        Hide
        mikemccand Michael McCandless added a comment -

        > I'm not enough of an expert to know if this would be faster or slower than a simple fileMap==null check.
        > I would guess it depends on what needs to be set up in the stack frame to potentially catch an exception, and if the try/catch/throw code prevents any optimizations (such as inlining).
        >
        > Also, catching a NPE seems a little icky to me... so w/o more info I'd lean toward a fileMap==null check (if anything).

        OK, good points. It is not in fact clear that a try/catch solution is
        less costly, so, let's keep it simple. I will just add ensureOpen().

        Show
        mikemccand Michael McCandless added a comment - > I'm not enough of an expert to know if this would be faster or slower than a simple fileMap==null check. > I would guess it depends on what needs to be set up in the stack frame to potentially catch an exception, and if the try/catch/throw code prevents any optimizations (such as inlining). > > Also, catching a NPE seems a little icky to me... so w/o more info I'd lean toward a fileMap==null check (if anything). OK, good points. It is not in fact clear that a try/catch solution is less costly, so, let's keep it simple. I will just add ensureOpen().
        Hide
        mikemccand Michael McCandless added a comment -

        Attached patch: removed AlreadyClosedException from throws clauses & javadocs; added ensureOpen calls inside RAMDirectory as well.

        Show
        mikemccand Michael McCandless added a comment - Attached patch: removed AlreadyClosedException from throws clauses & javadocs; added ensureOpen calls inside RAMDirectory as well.

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            mikemccand Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development