Lucene - Core
  1. Lucene - Core
  2. LUCENE-5906

Use Files.delete instead of File.delete + made up exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      We are on java7, if we cannot delete a file, this one returns a real exception as to why.

      1. LUCENE-5906.patch
        94 kB
        Robert Muir
      2. LUCENE-5906.patch
        92 kB
        Robert Muir
      3. LUCENE-5906.patch
        92 kB
        Robert Muir
      4. LUCENE-5906.patch
        91 kB
        Robert Muir
      5. LUCENE-5906.patch
        1 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        start of a patch. i just fixed FSDirectory. We should probably look for more places.

        Show
        Robert Muir added a comment - start of a patch. i just fixed FSDirectory. We should probably look for more places.
        Hide
        Uwe Schindler added a comment -

        Yeah, we should fix this!

        In the future we can also start to cut over everything to Path, because we can use "virtual filesystems" after that (e.g. put an index into a readonly ZIP file and let FSDirectory open it from inside the ZIP file using a Zip Filesystem Path - this may be useful for some - maybe...).

        In my opinion, we should also add the "old" methods to forbidden-apis. And much later put whole "java.io.File" into forbidden!

        Show
        Uwe Schindler added a comment - Yeah, we should fix this! In the future we can also start to cut over everything to Path, because we can use "virtual filesystems" after that (e.g. put an index into a readonly ZIP file and let FSDirectory open it from inside the ZIP file using a Zip Filesystem Path - this may be useful for some - maybe...). In my opinion, we should also add the "old" methods to forbidden-apis. And much later put whole "java.io.File" into forbidden!
        Hide
        Robert Muir added a comment -

        Updated patch, with full cutover to Files.delete() (and File.delete() banned in forbidden apis).

        This forced me to review all code deleting files, and I had to plug some leaks in suggesters, hunspell, offline sorter.

        Additionally to prevent such leaks, i had to add some IOUtils methods like deleteFilesIgnoringExceptions and so on.

        I moved TestUtil.rm to IOUtils, and fixed its exception messaging to return the actual exception messages for each file it couldnt delete. This is now used several places in source too (that had their own duplicate "rm" methods).

        I think we should do this as a step, because deleting files is about the most dangerous thing our code can do. So we really need real exceptions here in case something goes bad.

        Later we should investigate what Uwe suggests (Path api). This would be good to look at for lucene 5, but this patch is heavy enough.

        Show
        Robert Muir added a comment - Updated patch, with full cutover to Files.delete() (and File.delete() banned in forbidden apis). This forced me to review all code deleting files, and I had to plug some leaks in suggesters, hunspell, offline sorter. Additionally to prevent such leaks, i had to add some IOUtils methods like deleteFilesIgnoringExceptions and so on. I moved TestUtil.rm to IOUtils, and fixed its exception messaging to return the actual exception messages for each file it couldnt delete. This is now used several places in source too (that had their own duplicate "rm" methods). I think we should do this as a step, because deleting files is about the most dangerous thing our code can do. So we really need real exceptions here in case something goes bad. Later we should investigate what Uwe suggests (Path api). This would be good to look at for lucene 5, but this patch is heavy enough.
        Hide
        Michael McCandless added a comment -

        Wow, this is great: +1 to commit. Crazy all the places in our sources that we had for deleting files...

        Show
        Michael McCandless added a comment - Wow, this is great: +1 to commit. Crazy all the places in our sources that we had for deleting files...
        Hide
        Uwe Schindler added a comment -

        Maybe we should refactor the IOUtils a bit and provide varargs variants that simply delegate to Arrays.asList(varargs).

        We should also cleanup other code duplication in IOUtils and solely use asList()!

        Show
        Uwe Schindler added a comment - Maybe we should refactor the IOUtils a bit and provide varargs variants that simply delegate to Arrays.asList(varargs) . We should also cleanup other code duplication in IOUtils and solely use asList()!
        Hide
        Robert Muir added a comment -

        Yeah, lemme try to fix that.

        At first, when looking at lucene-core, i didnt think even IOUtils methods for deleting were justified. Then i saw how many places were doing this, but tried to keep the minimal set here. And you are right, i didnt want to duplicate with varargs. But we should just use asList to avoid this (in close, too).

        Show
        Robert Muir added a comment - Yeah, lemme try to fix that. At first, when looking at lucene-core, i didnt think even IOUtils methods for deleting were justified. Then i saw how many places were doing this, but tried to keep the minimal set here. And you are right, i didnt want to duplicate with varargs. But we should just use asList to avoid this (in close, too).
        Hide
        Robert Muir added a comment -

        Updated patch removing Arrays.asList/singleton stuff with varargs sugar instead.

        Show
        Robert Muir added a comment - Updated patch removing Arrays.asList/singleton stuff with varargs sugar instead.
        Hide
        Uwe Schindler added a comment -

        +1, thanks!

        Show
        Uwe Schindler added a comment - +1, thanks!
        Hide
        Uwe Schindler added a comment -

        For me Lucene tests worked in windows.

        Solr failed one test:

           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestArbitraryIndexDir -Dtests.method=testLoadNewIndexDir -Dtests.seed=269DADB350875937 -Dtests.directory=NIOFSDirectory -Dtests.locale=ro_RO -Dtests.timezone=PST -Dtests.file.encoding=UTF-8
           [junit4] ERROR   1.55s J1 | TestArbitraryIndexDir.testLoadNewIndexDir <<<
           [junit4]    > Throwable #1: java.io.IOException: Could not remove the following files (in the order of attempts):
           [junit4]    >    C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J1\.\temp\solr.core.TestArbitraryIndexDir-269DADB350875937-001\tempDir-001\index_temp: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J1\.\temp\solr.core.TestArbitraryIndexDir-269DADB350875937-001\tempDir-001\index_temp
           [junit4]    >        at __randomizedtesting.SeedInfo.seed([269DADB350875937:CFC7168BCE1EC99F]:0)
           [junit4]    >        at org.apache.lucene.util.IOUtils.rm(IOUtils.java:315)
           [junit4]    >        at org.apache.solr.core.TestArbitraryIndexDir.testLoadNewIndexDir(TestArbitraryIndexDir.java:135)
           [junit4]    >        at java.lang.Thread.run(Thread.java:745)
        
        Show
        Uwe Schindler added a comment - For me Lucene tests worked in windows. Solr failed one test: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestArbitraryIndexDir -Dtests.method=testLoadNewIndexDir -Dtests.seed=269DADB350875937 -Dtests.directory=NIOFSDirectory -Dtests.locale=ro_RO -Dtests.timezone=PST -Dtests.file.encoding=UTF-8 [junit4] ERROR 1.55s J1 | TestArbitraryIndexDir.testLoadNewIndexDir <<< [junit4] > Throwable #1: java.io.IOException: Could not remove the following files (in the order of attempts): [junit4] > C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J1\.\temp\solr.core.TestArbitraryIndexDir-269DADB350875937-001\tempDir-001\index_temp: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J1\.\temp\solr.core.TestArbitraryIndexDir-269DADB350875937-001\tempDir-001\index_temp [junit4] > at __randomizedtesting.SeedInfo.seed([269DADB350875937:CFC7168BCE1EC99F]:0) [junit4] > at org.apache.lucene.util.IOUtils.rm(IOUtils.java:315) [junit4] > at org.apache.solr.core.TestArbitraryIndexDir.testLoadNewIndexDir(TestArbitraryIndexDir.java:135) [junit4] > at java.lang.Thread.run(Thread.java:745)
        Hide
        Uwe Schindler added a comment -

        This reproduces, it is not caused by virus scanner!

        The problem here is two:

        • the Exception message is broken
        • the test is broken on windows (should reproduce on jenkins)
        Show
        Uwe Schindler added a comment - This reproduces, it is not caused by virus scanner! The problem here is two: the Exception message is broken the test is broken on windows (should reproduce on jenkins)
        Hide
        Robert Muir added a comment -

        Previously the test just ignored the return value from delete.

        Maybe it doesn't need to delete at all. But i saw this newDir created as:

        File newDir = new File(h.getCore().getDataDir() + "index_temp");
        

        So I was afraid to remove the delete() (and replaced it with a picky one), because i don't know if this directory is a child of one called from LuceneTestCase (which is guaranteed to be cleaned up correctly) or instead created in some arbitrary way.

        Show
        Robert Muir added a comment - Previously the test just ignored the return value from delete. Maybe it doesn't need to delete at all. But i saw this newDir created as: File newDir = new File(h.getCore().getDataDir() + "index_temp" ); So I was afraid to remove the delete() (and replaced it with a picky one), because i don't know if this directory is a child of one called from LuceneTestCase (which is guaranteed to be cleaned up correctly) or instead created in some arbitrary way.
        Hide
        Uwe Schindler added a comment -

        The harrness "h" should create a new autocleaned directory. I think in this test just one of the file in there keeps open.

        Show
        Uwe Schindler added a comment - The harrness "h" should create a new autocleaned directory. I think in this test just one of the file in there keeps open.
        Hide
        Robert Muir added a comment -

        I removed this delete (its unnecessary, if it wants to do it, should be in tearDown anyway, and test cleanup will take care correctly).

        I also fixed exception messages for IOUtils.rm, thanks!

        Show
        Robert Muir added a comment - I removed this delete (its unnecessary, if it wants to do it, should be in tearDown anyway, and test cleanup will take care correctly). I also fixed exception messages for IOUtils.rm, thanks!
        Hide
        Uwe Schindler added a comment -

        New patch fails in datacorrumpterhandler:

           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestNonWritablePersistFile -Dtests.method=testNonWritablePersistFile -Dtests.seed=CA8991891E1876AB -Dtests.locale=mt -Dtests.timezone=Asia/Tokyo -Dtests.file.encoding=UTF-8
           [junit4] ERROR   0.55s J2 | TestNonWritablePersistFile.testNonWritablePersistFile <<<
           [junit4]    > Throwable #1: java.nio.file.AccessDeniedException: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\contrib\solr-dataimporthandler\test\J2\.\temp\solr.handler.dataimport.TestNonWritablePersistFile-CA8991891E1876AB-001\tempDir-001\collection1\conf\dataimport.properties
           [junit4]    >        at __randomizedtesting.SeedInfo.seed([CA8991891E1876AB:7C810C466217C87E]:0)
           [junit4]    >        at sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:83)
           [junit4]    >        at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97)
           [junit4]    >        at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102)
           [junit4]    >        at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:269)
           [junit4]    >        at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
           [junit4]    >        at java.nio.file.Files.delete(Files.java:1079)
           [junit4]    >        at org.apache.solr.handler.dataimport.TestNonWritablePersistFile.testNonWritablePersistFile(TestNonWritablePersistFile.java:96)
           [junit4]    >        at java.lang.Thread.run(Thread.java:745)
        
        Show
        Uwe Schindler added a comment - New patch fails in datacorrumpterhandler: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestNonWritablePersistFile -Dtests.method=testNonWritablePersistFile -Dtests.seed=CA8991891E1876AB -Dtests.locale=mt -Dtests.timezone=Asia/Tokyo -Dtests.file.encoding=UTF-8 [junit4] ERROR 0.55s J2 | TestNonWritablePersistFile.testNonWritablePersistFile <<< [junit4] > Throwable #1: java.nio.file.AccessDeniedException: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\contrib\solr-dataimporthandler\test\J2\.\temp\solr.handler.dataimport.TestNonWritablePersistFile-CA8991891E1876AB-001\tempDir-001\collection1\conf\dataimport.properties [junit4] > at __randomizedtesting.SeedInfo.seed([CA8991891E1876AB:7C810C466217C87E]:0) [junit4] > at sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:83) [junit4] > at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97) [junit4] > at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102) [junit4] > at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:269) [junit4] > at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103) [junit4] > at java.nio.file.Files.delete(Files.java:1079) [junit4] > at org.apache.solr.handler.dataimport.TestNonWritablePersistFile.testNonWritablePersistFile(TestNonWritablePersistFile.java:96) [junit4] > at java.lang.Thread.run(Thread.java:745)
        Hide
        Robert Muir added a comment -

        Updated patch, same issue as the other one. Thanks Uwe

        Show
        Robert Muir added a comment - Updated patch, same issue as the other one. Thanks Uwe
        Hide
        Uwe Schindler added a comment -

        Hi the patch did not pass, too. The problem here is simple: the file was made read-only, so cannot be deleted. You need to add f.setWritable(true); at the end of test.

        Otherwise it passes on windows, I ran the whole test suite ignoring failures and this was the only one broken.

        Show
        Uwe Schindler added a comment - Hi the patch did not pass, too. The problem here is simple: the file was made read-only, so cannot be deleted. You need to add f.setWritable(true); at the end of test. Otherwise it passes on windows, I ran the whole test suite ignoring failures and this was the only one broken.
        Hide
        Robert Muir added a comment -

        Thanks for debugging this Uwe! I'll add setWriteable and commit.

        Show
        Robert Muir added a comment - Thanks for debugging this Uwe! I'll add setWriteable and commit.
        Hide
        ASF subversion and git services added a comment -

        Commit 1622506 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1622506 ]

        LUCENE-5906: Use Files.delete everywhere instead of File.delete

        Show
        ASF subversion and git services added a comment - Commit 1622506 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1622506 ] LUCENE-5906 : Use Files.delete everywhere instead of File.delete
        Hide
        ASF subversion and git services added a comment -

        Commit 1622511 from Robert Muir in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1622511 ]

        LUCENE-5906: Use Files.delete everywhere instead of File.delete

        Show
        ASF subversion and git services added a comment - Commit 1622511 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1622511 ] LUCENE-5906 : Use Files.delete everywhere instead of File.delete
        Hide
        Robert Muir added a comment -

        Thanks Uwe for all the testing and debugging on windows!

        Show
        Robert Muir added a comment - Thanks Uwe for all the testing and debugging on windows!
        Hide
        Hoss Man added a comment -

        Looks like a backport problem...

        -check-forbidden-all:
        [forbidden-apis] Reading bundled API signatures: jdk-unsafe-1.7
        [forbidden-apis] Reading bundled API signatures: jdk-deprecated-1.7
        [forbidden-apis] Reading API signatures: /home/hossman/lucene/4x_dev/lucene/tools/forbiddenApis/base.txt
        [forbidden-apis] Loading classes to check...
        [forbidden-apis] Scanning for API signatures and dependencies...
        [forbidden-apis] Forbidden method invocation: java.io.File#delete() [use Files.delete for real exception, IOUtils.deleteFilesIgnoringExceptions if you dont care]
        [forbidden-apis]   in org.apache.lucene.benchmark.byTask.utils.FileUtils (FileUtils.java:41)
        [forbidden-apis] Forbidden method invocation: java.io.File#delete() [use Files.delete for real exception, IOUtils.deleteFilesIgnoringExceptions if you dont care]
        [forbidden-apis]   in org.apache.lucene.benchmark.byTask.utils.FileUtils (FileUtils.java:51)
        [forbidden-apis] Scanned 210 (and 445 related) class file(s) for forbidden API invocations (in 0.09s), 2 error(s).
        

        org.apache.lucene.benchmark.byTask.utils.FileUtils doesn't seem to exist in trunk, so it was probably overlooked in the original patch & backport

        Show
        Hoss Man added a comment - Looks like a backport problem... -check-forbidden-all: [forbidden-apis] Reading bundled API signatures: jdk-unsafe-1.7 [forbidden-apis] Reading bundled API signatures: jdk-deprecated-1.7 [forbidden-apis] Reading API signatures: /home/hossman/lucene/4x_dev/lucene/tools/forbiddenApis/base.txt [forbidden-apis] Loading classes to check... [forbidden-apis] Scanning for API signatures and dependencies... [forbidden-apis] Forbidden method invocation: java.io.File#delete() [use Files.delete for real exception, IOUtils.deleteFilesIgnoringExceptions if you dont care] [forbidden-apis] in org.apache.lucene.benchmark.byTask.utils.FileUtils (FileUtils.java:41) [forbidden-apis] Forbidden method invocation: java.io.File#delete() [use Files.delete for real exception, IOUtils.deleteFilesIgnoringExceptions if you dont care] [forbidden-apis] in org.apache.lucene.benchmark.byTask.utils.FileUtils (FileUtils.java:51) [forbidden-apis] Scanned 210 (and 445 related) class file(s) for forbidden API invocations (in 0.09s), 2 error(s). org.apache.lucene.benchmark.byTask.utils.FileUtils doesn't seem to exist in trunk, so it was probably overlooked in the original patch & backport
        Hide
        Hoss Man added a comment -

        bah ... somehow i have an inconsistent checkout?

        sorry for the noise

        Show
        Hoss Man added a comment - bah ... somehow i have an inconsistent checkout? sorry for the noise
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development