LUCENE-8318: Ensure pending delete is not brought back on a try delete attempt

When renaming a file, `FSDirectory#rename` tries to delete the dest file
if it's in the pending deletes list. If that delete fails, it adds the
dest to the pending deletes list again. This causes the dest file to be
deleted later by `deletePendingFiles`.
This commit is contained in:
Simon Willnauer 2018-05-17 09:15:31 +02:00
parent 0159e4b974
commit 3fe612bed2
3 changed files with 73 additions and 7 deletions

View File

@ -250,6 +250,7 @@ public abstract class FSDirectory extends BaseDirectory {
// If this file was pending delete, we are now bringing it back to life:
if (pendingDeletes.remove(name)) {
privateDeleteFile(name, true); // try again to delete it - this is best effort
pendingDeletes.remove(name); // watch out - if the delete fails it put
}
return new FSIndexOutput(name);
}
@ -297,6 +298,7 @@ public abstract class FSDirectory extends BaseDirectory {
maybeDeletePendingFiles();
if (pendingDeletes.remove(dest)) {
privateDeleteFile(dest, true); // try again to delete it - this is best effort
pendingDeletes.remove(dest); // watch out if the delete fails it's back in here.
}
Files.move(directory.resolve(source), directory.resolve(dest), StandardCopyOption.ATOMIC_MOVE);
}

View File

@ -18,8 +18,14 @@ package org.apache.lucene.store;
import java.io.IOException;
import java.net.URI;
import java.nio.file.FileSystem;
import java.nio.file.Path;
import org.apache.lucene.mockfile.FilterPath;
import org.apache.lucene.mockfile.WindowsFS;
import org.apache.lucene.util.IOUtils;
/**
* Tests SimpleFSDirectory
*/
@ -29,4 +35,46 @@ public class TestSimpleFSDirectory extends BaseDirectoryTestCase {
protected Directory getDirectory(Path path) throws IOException {
return new SimpleFSDirectory(path);
}
public void testRenameWithPendingDeletes() throws IOException {
Path path = createTempDir();
// Use WindowsFS to prevent open files from being deleted:
FileSystem fs = new WindowsFS(path.getFileSystem()).getFileSystem(URI.create("file:///"));
Path root = new FilterPath(path, fs);
Directory directory = getDirectory(root);
IndexOutput output = directory.createOutput("target.txt", IOContext.DEFAULT);
output.writeInt(1);
output.close();
IndexOutput output1 = directory.createOutput("source.txt", IOContext.DEFAULT);
output1.writeInt(2);
output1.close();
IndexInput input = directory.openInput("target.txt", IOContext.DEFAULT);
directory.deleteFile("target.txt");
directory.rename("source.txt", "target.txt");
IndexInput input1 = directory.openInput("target.txt", IOContext.DEFAULT);
assertTrue(directory.getPendingDeletions().isEmpty());
assertEquals(1, input.readInt());
assertEquals(2, input1.readInt());
IOUtils.close(input1, input, directory);
}
public void testCreateOutputWithPendingDeletes() throws IOException {
Path path = createTempDir();
// Use WindowsFS to prevent open files from being deleted:
FileSystem fs = new WindowsFS(path.getFileSystem()).getFileSystem(URI.create("file:///"));
Path root = new FilterPath(path, fs);
Directory directory = getDirectory(root);
IndexOutput output = directory.createOutput("file.txt", IOContext.DEFAULT);
output.writeInt(1);
output.close();
IndexInput input = directory.openInput("file.txt", IOContext.DEFAULT);
directory.deleteFile("file.txt");
expectThrows(IOException.class, () -> {
directory.createOutput("file.txt", IOContext.DEFAULT);
});
assertTrue(directory.getPendingDeletions().isEmpty());
assertEquals(1, input.readInt());
IOUtils.close(input, directory);
}
}

View File

@ -89,18 +89,21 @@ public class WindowsFS extends HandleTrackingFS {
}
}
private Object getKeyOrNull(Path path) {
try {
return getKey(path);
} catch (Exception ignore) {
// we don't care if the file doesn't exist
}
return null;
}
/**
* Checks that it's ok to delete {@code Path}. If the file
* is still open, it throws IOException("access denied").
*/
private void checkDeleteAccess(Path path) throws IOException {
Object key = null;
try {
key = getKey(path);
} catch (Throwable ignore) {
// we don't care if the file doesn't exist
}
Object key = getKeyOrNull(path);
if (key != null) {
synchronized(openFiles) {
if (openFiles.containsKey(key)) {
@ -122,7 +125,20 @@ public class WindowsFS extends HandleTrackingFS {
public void move(Path source, Path target, CopyOption... options) throws IOException {
synchronized (openFiles) {
checkDeleteAccess(source);
Object key = getKeyOrNull(target);
super.move(source, target, options);
if (key != null) {
Object newKey = getKey(target);
if (newKey.equals(key) == false) {
// we need to transfer ownership here if we have open files on this file since the getKey() method will
// return a different i-node next time we call it with the target path and our onClose method will
// trip an assert
Integer remove = openFiles.remove(key);
if (remove != null) {
openFiles.put(newKey, remove);
}
}
}
}
}