Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.1, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      In LUCENE-2471 we were discussing the copyBytes issue, and Shai and I had a discussion about how we could prevent such bugs in the future.

      One thing that lead to the bug existing in our code for so long, was that it only happened on windows (e.g. never failed in hudson!)
      This was because the bug only happened if you were copying from SimpleFSDirectory, and the test used FSDirectory.open

      Today the situation is improving: most tests use newDirectory() which is random by default and never use FSDir.open,
      it always uses SimpleFS or NIOFS so that the same random seed will reproduce across both windows and unix.

      So I think we need to review all uses of FSDirectory.open in our tests, and minimize these.
      In general tests should use newDirectory().
      If the test comes with say a zip-file and wants to explicitly open stuff from disk, I think it should open the contents with say SimpleFSDir,
      and then call newDirectory(Directory) to copy into a new "random" implementation for actual testing. This method already exists:

        /**
         * Returns a new Dictionary instance, with contents copied from the
         * provided directory. See {@link #newDirectory()} for more
         * information.
         */
        public static MockDirectoryWrapper newDirectory(Directory d) throws IOException {
      

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          I think that newDirectory(Directory) is weird. I'd think we'll have a newDirectory(File) which will either return a FSDir on that File, or a RAMDir where the files are copied into it.

          Of all the tests we have, only one calls this method and from what I can tell, for no good reason. The method just copies the incoming Dir to another one, an operation I think is useless for tests purposes.

          Show
          Shai Erera added a comment - I think that newDirectory(Directory) is weird. I'd think we'll have a newDirectory(File) which will either return a FSDir on that File, or a RAMDir where the files are copied into it. Of all the tests we have, only one calls this method and from what I can tell, for no good reason. The method just copies the incoming Dir to another one, an operation I think is useless for tests purposes.
          Hide
          Robert Muir added a comment -

          Of all the tests we have, only one calls this method and from what I can tell, for no good reason. The method just copies the incoming Dir to another one, an operation I think is useless for tests purposes.

          Let me explain why this is: there are a number of tests that:

          1. build up a dir with IW
          2. in a loop, copy the dir into several other dir(s) and do scary tests

          when i converted things to newDirectory, i only did the "easy tests". I think i did one of these more scary ones. We should do the others.
          I think it can make sense for a test to do this, if it wants to look for some race condition or something and doesnt want to re-index into a new dir in each loop.

          However...

          I think that newDirectory(Directory) is weird. I'd think we'll have a newDirectory(File) which will either return a FSDir on that File, or a RAMDir where the files are copied into it.

          I think newDirectory(File) is a good idea! But i still think we should keep the other one. So newDirectory(File) could be done with:

          SimpleFSDirectory fs = new SimpleFSDirectory(file);
          Directory ret = newDirectory(fs);
          fs.close();
          return ret;
          
          Show
          Robert Muir added a comment - Of all the tests we have, only one calls this method and from what I can tell, for no good reason. The method just copies the incoming Dir to another one, an operation I think is useless for tests purposes. Let me explain why this is: there are a number of tests that: build up a dir with IW in a loop, copy the dir into several other dir(s) and do scary tests when i converted things to newDirectory, i only did the "easy tests". I think i did one of these more scary ones. We should do the others. I think it can make sense for a test to do this, if it wants to look for some race condition or something and doesnt want to re-index into a new dir in each loop. However... I think that newDirectory(Directory) is weird. I'd think we'll have a newDirectory(File) which will either return a FSDir on that File, or a RAMDir where the files are copied into it. I think newDirectory(File) is a good idea! But i still think we should keep the other one. So newDirectory(File) could be done with: SimpleFSDirectory fs = new SimpleFSDirectory(file); Directory ret = newDirectory(fs); fs.close(); return ret;
          Hide
          Shai Erera added a comment -

          Ok I agree. Currently there is only one 'testEmptyIndex()' test that makes use of newDirectory(Directory). But perhaps after we've converted more tests to not use FSDirectory.open, more will use it.

          Show
          Shai Erera added a comment - Ok I agree. Currently there is only one 'testEmptyIndex()' test that makes use of newDirectory(Directory). But perhaps after we've converted more tests to not use FSDirectory.open, more will use it.
          Hide
          Shai Erera added a comment -

          Started to go over the tests, and I think that newDirectory(File) could be useful to return any of the FSDirectory impls, over that File, and not copy it to a random directory. Tests like LockFactory stress tests etc need to work on a FSDirectory impl so either we make sure the test runs on all of them, or we introduce newDirectory(File) which draws one of the FSDirs.

          Show
          Shai Erera added a comment - Started to go over the tests, and I think that newDirectory(File) could be useful to return any of the FSDirectory impls, over that File, and not copy it to a random directory. Tests like LockFactory stress tests etc need to work on a FSDirectory impl so either we make sure the test runs on all of them, or we introduce newDirectory(File) which draws one of the FSDirs.
          Hide
          Shai Erera added a comment -

          Patch fixes all tests to use newDirectory() or newFSDirectory(), except for backwards test code. Also some improvements to LuceneTestCase are included (in the scope of the issue).

          All tests pass.

          Note that I had to impl setLockFactory by FSDirectory, which caused the addition of "throws IOException" to Directory.setLockFactory. The changes in the issue revealed an inconsistency we had - previously if you "new FSDirectory(File, LockFactory)" or "new FSDirectory(File).setLockFactory" you wouldn't get the same behavior. The former set the lockPrefix to null while the latter didn't. The changes in the patch uncovered that (TestLockFactory) and so I made the change.

          While technically this is a backwards break, I think adding that 'throws' stuff to Directory is safe. Kinda hard to believe someone has code which calls dir.setLockFactory and does not handle/throw IOException. I spelled it out in CHANGES.

          Show
          Shai Erera added a comment - Patch fixes all tests to use newDirectory() or newFSDirectory(), except for backwards test code. Also some improvements to LuceneTestCase are included (in the scope of the issue). All tests pass. Note that I had to impl setLockFactory by FSDirectory, which caused the addition of "throws IOException" to Directory.setLockFactory. The changes in the issue revealed an inconsistency we had - previously if you "new FSDirectory(File, LockFactory)" or "new FSDirectory(File).setLockFactory" you wouldn't get the same behavior. The former set the lockPrefix to null while the latter didn't. The changes in the patch uncovered that (TestLockFactory) and so I made the change. While technically this is a backwards break, I think adding that 'throws' stuff to Directory is safe. Kinda hard to believe someone has code which calls dir.setLockFactory and does not handle/throw IOException. I spelled it out in CHANGES.
          Hide
          Robert Muir added a comment -

          looking through the patch, it looks very good to me!

          i agree with fixing FSDirectory's setLockFactory, and adding the throws IOException to the method signature.

          I think after this issue is resolved we should make a separate followup issue to create a MockDirectoryFactory (or whatever we wish to call it) for Solr
          that uses this new newFSDirectory() method, because currently Solr's tests are using FSDirectory.open I think, which is system-dependent.

          Show
          Robert Muir added a comment - looking through the patch, it looks very good to me! i agree with fixing FSDirectory's setLockFactory, and adding the throws IOException to the method signature. I think after this issue is resolved we should make a separate followup issue to create a MockDirectoryFactory (or whatever we wish to call it) for Solr that uses this new newFSDirectory() method, because currently Solr's tests are using FSDirectory.open I think, which is system-dependent.
          Hide
          Shai Erera added a comment -

          Note that I searched for FSDirectory.open in Solr's tests too, and found 3 tests that called it, and fixed them. But I agree this MockDirectoryFactory is needed too, since I assume other tests use a DirFactory outside the test code, which uses FSDirectory.open inside.

          One thing I discovered is that the Directory obtained from DirFactory.open() is not closed by Solr code. So I guess a lot of Solr tests would fail after we introduce MockDirFactory. If you wish, we can revert the fixes I did to the 3 tests, and fix them in the follow up issue. Currently, I had to hack one test to make sure the dir is closed in the end.

          Show
          Shai Erera added a comment - Note that I searched for FSDirectory.open in Solr's tests too, and found 3 tests that called it, and fixed them. But I agree this MockDirectoryFactory is needed too, since I assume other tests use a DirFactory outside the test code, which uses FSDirectory.open inside. One thing I discovered is that the Directory obtained from DirFactory.open() is not closed by Solr code. So I guess a lot of Solr tests would fail after we introduce MockDirFactory. If you wish, we can revert the fixes I did to the 3 tests, and fix them in the follow up issue. Currently, I had to hack one test to make sure the dir is closed in the end.
          Hide
          Robert Muir added a comment -

          One thing I discovered is that the Directory obtained from DirFactory.open() is not closed by Solr code. So I guess a lot of Solr tests would fail after we introduce MockDirFactory. If you wish, we can revert the fixes I did to the 3 tests, and fix them in the follow up issue. Currently, I had to hack one test to make sure the dir is closed in the end.

          Your call either way, but you are right... and once we fix whatever issue is causing directories not to be closed in Solr tests, we will still have more things to chase down.
          This is because MockDirectoryWrapper tracks open files, and when you call .close() on it, it will cause the test to fail if you didn't close things like IndexReaders.
          I know at the very least, there is an issue with SpellChecker: SOLR-1877.

          Worst case, we can setNoDeleteOpenFile(false) for Solr, which won't enforce this... but I'm not really positive this check should be tied to setNoDeleteOpenFile, maybe it should have its own setter, and I think even in the case where we don't want it to fail (intentional resource leak, I think TestCrash/*OnDiskFull), we should have it warn
          that there is a resource leak.

          Show
          Robert Muir added a comment - One thing I discovered is that the Directory obtained from DirFactory.open() is not closed by Solr code. So I guess a lot of Solr tests would fail after we introduce MockDirFactory. If you wish, we can revert the fixes I did to the 3 tests, and fix them in the follow up issue. Currently, I had to hack one test to make sure the dir is closed in the end. Your call either way, but you are right... and once we fix whatever issue is causing directories not to be closed in Solr tests, we will still have more things to chase down. This is because MockDirectoryWrapper tracks open files, and when you call .close() on it, it will cause the test to fail if you didn't close things like IndexReaders. I know at the very least, there is an issue with SpellChecker: SOLR-1877 . Worst case, we can setNoDeleteOpenFile(false) for Solr, which won't enforce this... but I'm not really positive this check should be tied to setNoDeleteOpenFile, maybe it should have its own setter, and I think even in the case where we don't want it to fail (intentional resource leak, I think TestCrash/*OnDiskFull), we should have it warn that there is a resource leak.
          Hide
          Shai Erera added a comment -

          Except for one class, the rest of the fixes are straightforward, so let's keep them here. And I think we can keep the other fix as well, w/ a TODO to remove it once MockDirFactory is in place (and perhaps make some changes to that TestDirFactory).

          Show
          Shai Erera added a comment - Except for one class, the rest of the fixes are straightforward, so let's keep them here. And I think we can keep the other fix as well, w/ a TODO to remove it once MockDirFactory is in place (and perhaps make some changes to that TestDirFactory).
          Hide
          Shai Erera added a comment -

          Committed revision 1043979 (3x). Now working to port it to trunk

          Show
          Shai Erera added a comment - Committed revision 1043979 (3x). Now working to port it to trunk
          Hide
          Shai Erera added a comment -

          Committed revision 1044008 (trunk).

          Thanks Robert !

          Show
          Shai Erera added a comment - Committed revision 1044008 (trunk). Thanks Robert !
          Hide
          Grant Ingersoll added a comment -

          Bulk close for 3.1

          Show
          Grant Ingersoll added a comment - Bulk close for 3.1

            People

            • Assignee:
              Unassigned
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development