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

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!

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.

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

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()!

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).

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

+1, thanks!

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)

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)

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.

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

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!

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)

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

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.

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

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

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

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

Thanks Uwe for all the testing and debugging on windows!

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

bah ... somehow i have an inconsistent checkout?

sorry for the noise

Bulk close after 5.0 release.