Lucene - Core
  1. Lucene - Core
  2. LUCENE-3418

Lucene is not fsync'ing files on commit

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.1, 3.2, 3.3, 3.4, 4.0-ALPHA
    • Fix Version/s: 3.4, 4.0-ALPHA
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Thanks to hurricane Irene, when Mark's electricity became unreliable, he discovered that on power loss Lucene could easily corrumpt the index, which of course should never happen...

      I was able to easily repro, by pulling the plug on an Ubuntu box during indexing. On digging, I discovered, to my horror, that Lucene is failing to fsync any files, ever!

      This bug was unfortunately created when we committed LUCENE-2328... that issue added tracking, in FSDir, of which files have been closed but not sync'd, so that when sync is called during IW.commit we only sync those files that haven't already been sync'd.

      That tracking is done via the FSDir.onIndexOutputClosed callback, called when an FSIndexOutput is closed. The bug is that we only call it on exception during close:

          @Override
          public void close() throws IOException {
            // only close the file if it has not been closed yet
            if (isOpen) {
              boolean success = false;
              try {
                super.close();
                success = true;
              } finally {
                isOpen = false;
                if (!success) {
                  try {
                    file.close();
                    parent.onIndexOutputClosed(this);
                  } catch (Throwable t) {
                    // Suppress so we don't mask original exception
                  }
                } else
                  file.close();
              }
            }
          }
      

      And so FSDir thinks no files need syncing when its sync method is called....

      I think instead we should call it up-front; better to over-sync than under-sync.

      The fix is trivial (move the callback up-front), but I'd love to somehow have a test that can catch such a bad regression in the future.... still I think we can do that test separately and commit this fix first.

      Note that even though LUCENE-2328 was backported to 2.9.x and 3.0.x, this bug wasn't, ie the backport was a much simpler fix (to just address the original memory leak); it's 3.1, 3.2, 3.3 and trunk when this bug is present.

        Activity

        Hide
        Shai Erera added a comment -

        Mike, just to make sure, did you actually see this leads to corruption, or you only suspect it? I ask because a while ago we opened LUCENE-3237 because we didn't think that FSDir.sync() works at all.

        Your fix is valid anyway, because if we'll fix sync(), it's good to know that now we always call it.

        Show
        Shai Erera added a comment - Mike, just to make sure, did you actually see this leads to corruption, or you only suspect it? I ask because a while ago we opened LUCENE-3237 because we didn't think that FSDir.sync() works at all. Your fix is valid anyway, because if we'll fix sync(), it's good to know that now we always call it.
        Hide
        Robert Muir added a comment -

        but I'd love to somehow have a test that can catch such a bad regression in the future

        Maybe we can come up with a test for this similar to the 'test' on HDFS-970.

        For example maybe we could have a freebsd-specific twist on that, that we run in our nightly
        build or something that uses geom/gnop or iscsi+firewalling or whatever our jail lets us
        get away with.

        failing that we could try to come up with a linux version but we would have to test it
        ourselves manually on a periodic basis.

        Show
        Robert Muir added a comment - but I'd love to somehow have a test that can catch such a bad regression in the future Maybe we can come up with a test for this similar to the 'test' on HDFS-970 . For example maybe we could have a freebsd-specific twist on that, that we run in our nightly build or something that uses geom/gnop or iscsi+firewalling or whatever our jail lets us get away with. failing that we could try to come up with a linux version but we would have to test it ourselves manually on a periodic basis.
        Hide
        Michael McCandless added a comment -

        Mike, just to make sure, did you actually see this leads to corruption, or you only suspect it?

        Mark and I saw real corruption just by pulling the power plug. Once
        the machine came back up there were a bunch of 0-length files, and the
        index was quite definitely corrupt It was trivial to reproduce. In
        one test, I watched 5 commits complete before cutting power and on
        reboot none of those commits were usable.

        But with the fix I committed I can no longer corrupt the index by
        pulling the plug.

        On LUCENE-3237: it still makes me somewhat nervous that we close the
        fd, time passes, open a new fd, fsync that. It would be "safer" if we
        fsync'd before closing, but this would be a challenge for Lucene.

        But the most recent POSIX standard (POSIX:2008) seem to suggest this
        is an OK approach:

        http://pubs.opengroup.org/onlinepubs/9699919799/functions/fsync.html

        Ie, if the system has _POSIX_SYNCHRONIZED_IO defined (I believe modern
        Linuxes do) then the semantics make it clear that the fsync applies to
        the underlying file and not just the bytes written to the fd you have
        open right now.

        Show
        Michael McCandless added a comment - Mike, just to make sure, did you actually see this leads to corruption, or you only suspect it? Mark and I saw real corruption just by pulling the power plug. Once the machine came back up there were a bunch of 0-length files, and the index was quite definitely corrupt It was trivial to reproduce. In one test, I watched 5 commits complete before cutting power and on reboot none of those commits were usable. But with the fix I committed I can no longer corrupt the index by pulling the plug. On LUCENE-3237 : it still makes me somewhat nervous that we close the fd, time passes, open a new fd, fsync that. It would be "safer" if we fsync'd before closing, but this would be a challenge for Lucene. But the most recent POSIX standard (POSIX:2008) seem to suggest this is an OK approach: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fsync.html Ie, if the system has _POSIX_SYNCHRONIZED_IO defined (I believe modern Linuxes do) then the semantics make it clear that the fsync applies to the underlying file and not just the bytes written to the fd you have open right now.
        Hide
        Michael McCandless added a comment -

        I think we should turnaround a 3.4.0 release with this fix. I'll build an RC.

        Show
        Michael McCandless added a comment - I think we should turnaround a 3.4.0 release with this fix. I'll build an RC.
        Hide
        Simon Willnauer added a comment -

        I think we should turnaround a 3.4.0 release with this fix. I'll build an RC.

        +1

        Show
        Simon Willnauer added a comment - I think we should turnaround a 3.4.0 release with this fix. I'll build an RC. +1
        Hide
        Mark Miller added a comment -

        I think we should turnaround a 3.4.0 release with this fix. I'll build an RC.

        +1

        Show
        Mark Miller added a comment - I think we should turnaround a 3.4.0 release with this fix. I'll build an RC. +1
        Hide
        Erick Erickson added a comment -

        File corruptions really scare me,
        +1

        Show
        Erick Erickson added a comment - File corruptions really scare me, +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1562418 from Michael McCandless in branch 'dev/branches/lucene5376'
        [ https://svn.apache.org/r1562418 ]

        LUCENE-3418: fix corner case: null disi means all docs didn't match

        Show
        ASF subversion and git services added a comment - Commit 1562418 from Michael McCandless in branch 'dev/branches/lucene5376' [ https://svn.apache.org/r1562418 ] LUCENE-3418 : fix corner case: null disi means all docs didn't match

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development