Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.1
    • Component/s: core/other
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: All

    • Bugzilla Id:
      24084

      Description

      The canonical form of a Java finalizer is:

      protected void finalize() throws Throwable()
      {
      try

      { // ... local code to finalize this class }

      catch (Throwable t)
      {
      }
      super.finalize(); // finalize base class.
      }

      The finalizers in IndexReader, IndexWriter, and FSDirectory don't conform. This
      is probably minor or null in effect, but the principle is important.

      As a matter of fact FSDirectory.finaliz() is entirely redundant and could be
      removed, as it doesn't do anything that RandomAccessFile.finalize would do
      automatically.

        Activity

        Hide
        salk31 Sam Hough added a comment -

        I think FSDirectory needs a finalize method adding to remove its reference
        from FSDirectory.DIRECTORIES otherwise, through normal garbage collection,
        directories could linger.

        I presume the orginator of this issue is commenting on the finalize methods for
        the Input and Output Streams.

        I'm assuming that the intention is for Lucene to clean up after itself even if close is
        not called explicitly. If this really is a bug then I'm happy to try and construct a unit
        test to check that FSDirectory cleans up after itself properly.

        Show
        salk31 Sam Hough added a comment - I think FSDirectory needs a finalize method adding to remove its reference from FSDirectory.DIRECTORIES otherwise, through normal garbage collection, directories could linger. I presume the orginator of this issue is commenting on the finalize methods for the Input and Output Streams. I'm assuming that the intention is for Lucene to clean up after itself even if close is not called explicitly. If this really is a bug then I'm happy to try and construct a unit test to check that FSDirectory cleans up after itself properly.
        Hide
        salk31 Sam Hough added a comment -

        Doh. Sorry. Been a long day. Finalize wont be called if DIRECTORIES still points at it Think twice, post once.

        Does this mean that clients of FSDirectory should have finalize methods that close the Directory?
        IndexReader.finalize for instance just cleans up its lock but doesn't call close()!?

        It is making my head hurt thinking back to C++ days of no automatic garbage collection.

        Sorry.

        Show
        salk31 Sam Hough added a comment - Doh. Sorry. Been a long day. Finalize wont be called if DIRECTORIES still points at it Think twice, post once. Does this mean that clients of FSDirectory should have finalize methods that close the Directory? IndexReader.finalize for instance just cleans up its lock but doesn't call close()!? It is making my head hurt thinking back to C++ days of no automatic garbage collection. Sorry.
        Hide
        esmond.pitt@bigpond.com Esmond Pitt added a comment -

        Apparently I haven't made myself clear. It is essential that any finalizer calls super.finalize() regardless of any exceptions it may encounter. The canonical form shown is one way of achieving this: another is finally

        { super.finalize(); }

        . The finalizers in the classes named do not conform to either pattern and hence whatever their base classes finalizers may do is not necessarily being done.

        Show
        esmond.pitt@bigpond.com Esmond Pitt added a comment - Apparently I haven't made myself clear. It is essential that any finalizer calls super.finalize() regardless of any exceptions it may encounter. The canonical form shown is one way of achieving this: another is finally { super.finalize(); } . The finalizers in the classes named do not conform to either pattern and hence whatever their base classes finalizers may do is not necessarily being done.
        Hide
        mikemccand Michael McCandless added a comment -

        OK I've fixed all cases of finalize except for one under contrib (contrib/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java) whose comment was spooky to me:

        // lucene >= 1.9 or lucene-1.4.3 with patch removing "final" in superclass
        protected void finalize() {}

        Thank you Esmond and sorry for the long delay!

        Show
        mikemccand Michael McCandless added a comment - OK I've fixed all cases of finalize except for one under contrib (contrib/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java) whose comment was spooky to me: // lucene >= 1.9 or lucene-1.4.3 with patch removing "final" in superclass protected void finalize() {} Thank you Esmond and sorry for the long delay!
        Hide
        whoschek wolfgang hoschek added a comment -

        Just to clarify: The empty finalize() method body in MemoryIndex measurabley improves performance of this class and it does not harm correctness because MemoryIndex does not require the superclass semantics wrt. concurrency.

        Show
        whoschek wolfgang hoschek added a comment - Just to clarify: The empty finalize() method body in MemoryIndex measurabley improves performance of this class and it does not harm correctness because MemoryIndex does not require the superclass semantics wrt. concurrency.
        Hide
        mikemccand Michael McCandless added a comment -

        Sneakiness. Thanks for clarifying!

        Show
        mikemccand Michael McCandless added a comment - Sneakiness. Thanks for clarifying!
        Hide
        mikemccand Michael McCandless added a comment -

        Closing all issues that were resolved for 2.1.

        Show
        mikemccand Michael McCandless added a comment - Closing all issues that were resolved for 2.1.

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            esmond.pitt@bigpond.com Esmond Pitt
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development