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

IndexWriter does not release its write lock when trying to open an index which does not yet exist

    Details

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

      Windows XP, Java 1.5, IntelliJ 6

    • Lucene Fields:
      New, Patch Available

      Description

      In version 2.0.0, the private IndexWriter constructor does not properly remove its write lock in the event of an error. This can be seen when one attempts to open (not create) an index in a directory which exists, but in which there is no segments file. Here is the offending code:

      247 private IndexWriter(Directory d, Analyzer a, final boolean create, boolean closeDir)
      248 throws IOException {
      249 this.closeDir = closeDir;
      250 directory = d;
      251 analyzer = a;
      252
      253 Lock writeLock = directory.makeLock(IndexWriter.WRITE_LOCK_NAME);
      254 if (!writeLock.obtain(writeLockTimeout)) // obtain write lock
      255 throw new IOException("Index locked for write: " + writeLock);
      256 this.writeLock = writeLock; // save it
      257
      258 synchronized (directory) { // in- & inter-process sync
      259 new Lock.With(directory.makeLock(IndexWriter.COMMIT_LOCK_NAME), commitLockTimeout) {
      260 public Object doBody() throws IOException

      { 261 if (create) 262 segmentInfos.write(directory); 263 else 264 segmentInfos.read(directory); 265 return null; 266 }

      267 }.run();
      268 }
      269 }

      On line 254, a write lock is obtained by the constructor. If an exception is raised inside the doBody() method (on line 260), then that exception is propagated, the constructor will fail, but the lock is not released until the object is garbage collected. This is typically an issue except when using the IndexModifier class.

      As of the filing of this bug, this has not yet been fixed in the trunk (IndexWriter.java#472959):

      251 private IndexWriter(Directory d, Analyzer a, final boolean create, boolean closeDir)
      252 throws IOException {
      253 this.closeDir = closeDir;
      254 directory = d;
      255 analyzer = a;
      256
      257 Lock writeLock = directory.makeLock(IndexWriter.WRITE_LOCK_NAME);
      258 if (!writeLock.obtain(writeLockTimeout)) // obtain write lock
      259 throw new IOException("Index locked for write: " + writeLock);
      260 this.writeLock = writeLock; // save it
      261
      262 synchronized (directory) { // in- & inter-process sync
      263 new Lock.With(directory.makeLock(IndexWriter.COMMIT_LOCK_NAME), commitLockTimeout) {
      264 public Object doBody() throws IOException

      { 265 if (create) 266 segmentInfos.write(directory); 267 else 268 segmentInfos.read(directory); 269 return null; 270 }

      271 }.run();
      272 }
      273 }

      1. LUCENE-715.patch
        5 kB
        Matthew Bogosian
      2. LUCENE-715.patch
        5 kB
        Matthew Bogosian

        Activity

        Hide
        mbogosian Matthew Bogosian added a comment -

        Attaching the fix as a patch. All tests pass. Patch includes fix, test case and modifications to the CHANGES.txt file.

        Show
        mbogosian Matthew Bogosian added a comment - Attaching the fix as a patch. All tests pass. Patch includes fix, test case and modifications to the CHANGES.txt file.
        Hide
        mbogosian Matthew Bogosian added a comment -

        In original description:

        s/except when using the IndexModifier class/when using the IndexModifier class/

        Show
        mbogosian Matthew Bogosian added a comment - In original description: s/except when using the IndexModifier class/when using the IndexModifier class/
        Hide
        mikemccand Michael McCandless added a comment -

        Good catch! And thanks for the patch. Plus I like the perl-ese correction above.

        It looks like with this patch you are swallowing an IOException if it happens in that try/catch block? So I think you should add a throw(e) at the end of the catch block? Also maybe move the try/catch outside the synchronized block? I usually try to minimize the code inside synchronized blocks, and I don't think the releasing of the writeLock on IOException needs to be inside the synchronized block.

        Show
        mikemccand Michael McCandless added a comment - Good catch! And thanks for the patch. Plus I like the perl-ese correction above. It looks like with this patch you are swallowing an IOException if it happens in that try/catch block? So I think you should add a throw(e) at the end of the catch block? Also maybe move the try/catch outside the synchronized block? I usually try to minimize the code inside synchronized blocks, and I don't think the releasing of the writeLock on IOException needs to be inside the synchronized block.
        Hide
        mbogosian Matthew Bogosian added a comment -

        Ugh! How embarrassing! If it makes you feel better, I re-raised the exception in my mind....

        I added the the missing throw statement and moved the try/catch outside of the synchronized block (which is probably even better since it will also take care of the case where the call to makeLock raises an IOException).

        Thanks for catching that (no pun intended)!

        Show
        mbogosian Matthew Bogosian added a comment - Ugh! How embarrassing! If it makes you feel better, I re-raised the exception in my mind.... I added the the missing throw statement and moved the try/catch outside of the synchronized block (which is probably even better since it will also take care of the case where the call to makeLock raises an IOException). Thanks for catching that (no pun intended)!
        Hide
        mikemccand Michael McCandless added a comment -

        Don't worry about it This is what open source is all about!

        OK this looks good to me. I will commit. Thanks Matthew, keep the patches coming!

        Show
        mikemccand Michael McCandless added a comment - Don't worry about it This is what open source is all about! OK this looks good to me. I will commit. Thanks Matthew, keep the patches coming!
        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:
            Unassigned
            Reporter:
            mbogosian Matthew Bogosian
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development