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

RAMDirectory.close() should have a comment about not releasing any resources

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: None
    • Component/s: core/store
    • Labels:
      None

      Description

      I wrongly assumed that calling RAMDirectory.close() would free up the memory occupied by the RAMDirectory.
      It might be helpful to add a javadoc comment that warns users that RAMDirectory.close() is a no-op, since it might be a common assumption that close() would release resources.

      1. ramdirectory.diff
        0.5 kB
        Nadav Har'El

        Activity

        Hide
        nyh Nadav Har'El added a comment -

        I propose a trivial patch, which does two very simple things:

        1. RAMDirectory.close(), instead of being a no-op, sets files=null. This allows the garbage collector to collect all the memory that was used for this RAMDirectory (this is the best we can do in Java).

        2. Make the documentation more explicit, from "Closes the store to future operations." to "Closes the store to future operations, releasing associated memory."

        Note that now, after a close(), any operations on the RAMDirectory will likely result in a NullPointerException. I don't think this matters, as the documentation clearly says that after a close() you're not allowed to operate on this object.

        Show
        nyh Nadav Har'El added a comment - I propose a trivial patch, which does two very simple things: 1. RAMDirectory.close(), instead of being a no-op, sets files=null. This allows the garbage collector to collect all the memory that was used for this RAMDirectory (this is the best we can do in Java). 2. Make the documentation more explicit, from "Closes the store to future operations." to "Closes the store to future operations, releasing associated memory." Note that now, after a close(), any operations on the RAMDirectory will likely result in a NullPointerException. I don't think this matters, as the documentation clearly says that after a close() you're not allowed to operate on this object.
        Hide
        hossman Hoss Man added a comment -

        Patch commited.

        Note that this patch caused an NPE in TestIndexWriterMerging.testLucene, but after reviewing the test I'm of the opinion that it made an invalid assumption about the order of events that was allowed, to the effect of...

        Directory merged = new RAMDirectory();
        ...
        merged.close();
        ...
        IndexReader reader = IndexReader.open(merged);

        ...I modified the test slightly to only close the directory once all uses of it were finished and included it in the commit.

        Show
        hossman Hoss Man added a comment - Patch commited. Note that this patch caused an NPE in TestIndexWriterMerging.testLucene, but after reviewing the test I'm of the opinion that it made an invalid assumption about the order of events that was allowed, to the effect of... Directory merged = new RAMDirectory(); ... merged.close(); ... IndexReader reader = IndexReader.open(merged); ...I modified the test slightly to only close the directory once all uses of it were finished and included it in the commit.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Just out of curiousity, does Lucene hold onto a RAMDirectory instance somewhere after it has called close on it? If so, that would have been the ideal place to fix it any kind of "leak".

        This patch, while not doing much harm, is not really "best practice" in Java and the existing RAMDirectory close() was correct.
        Am I missing something?

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Just out of curiousity, does Lucene hold onto a RAMDirectory instance somewhere after it has called close on it? If so, that would have been the ideal place to fix it any kind of "leak". This patch, while not doing much harm, is not really "best practice" in Java and the existing RAMDirectory close() was correct. Am I missing something?

          People

          • Assignee:
            hossman Hoss Man
            Reporter:
            heng.mei@gmail.com Heng Mei
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development