LUCENE-6499: WindowsFS misses to remove open file handle if file is concurrently deleted

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1681846 13f79535-47bb-0310-9956-ffa450edef68

lucene/test-framework/src/java/org/apache/lucene/mockfile/WindowsFS.java

@@ -17,15 +17,19 @@ package org.apache.lucene.mockfile;
  * limitations under the License.
  */
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.nio.file.CopyOption;
 import java.nio.file.FileSystem;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 /** 
  * FileSystem that (imperfectly) acts like windows. 
@@ -33,8 +37,7 @@ import java.util.Map;
  * Currently this filesystem only prevents deletion of open files.
  */
 public class WindowsFS extends HandleTrackingFS {
-  private final Map<Object,Integer> openFiles = new HashMap<>();
-  
+  final Map<Object,Integer> openFiles = new HashMap<>();
   // TODO: try to make this as realistic as possible... it depends e.g. how you
   // open files, if you map them, etc, if you can delete them (Uwe knows the rules)
   
@@ -60,8 +63,10 @@ public class WindowsFS extends HandleTrackingFS {
 
   @Override
   protected void onOpen(Path path, Object stream) throws IOException {
-    Object key = getKey(path);
     synchronized (openFiles) {
+      final Object key = getKey(path);
+      // we have to read the key under the lock otherwise me might leak the openFile handle
+      // if we concurrently delete or move this file.
       Integer v = openFiles.get(key);
       if (v != null) {
         v = Integer.valueOf(v.intValue()+1);
@@ -74,9 +79,10 @@ public class WindowsFS extends HandleTrackingFS {
 
   @Override
   protected void onClose(Path path, Object stream) throws IOException {
-    Object key = getKey(path);
+    Object key = getKey(path); // here we can read this outside of the lock
     synchronized (openFiles) {
       Integer v = openFiles.get(key);
+      assert v != null;
       if (v != null) {
         if (v.intValue() == 1) {
           openFiles.remove(key);
@@ -111,19 +117,25 @@ public class WindowsFS extends HandleTrackingFS {
 
   @Override
   public void delete(Path path) throws IOException {
+    synchronized (openFiles) {
       checkDeleteAccess(path);
       super.delete(path);
     }
+  }
 
   @Override
   public void move(Path source, Path target, CopyOption... options) throws IOException {
+    synchronized (openFiles) {
       checkDeleteAccess(source);
       super.move(source, target, options);
     }
+  }
 
   @Override
   public boolean deleteIfExists(Path path) throws IOException {
+    synchronized (openFiles) {
       checkDeleteAccess(path);
       return super.deleteIfExists(path);
     }
   }
+}

lucene/test-framework/src/test/org/apache/lucene/mockfile/TestWindowsFS.java

@@ -17,15 +17,25 @@ package org.apache.lucene.mockfile;
  * limitations under the License.
  */
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.lang.Exception;
+import java.lang.InterruptedException;
+import java.lang.NoSuchFieldException;
+import java.lang.RuntimeException;
 import java.net.URI;
 import java.nio.file.FileSystem;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.atomic.AtomicBoolean;
 
+import org.apache.lucene.mockfile.FilterPath;
+import org.apache.lucene.mockfile.WindowsFS;
 import org.apache.lucene.util.Constants;
 
 /** Basic tests for WindowsFS */
@@ -95,4 +105,57 @@ public class TestWindowsFS extends MockFileSystemTestCase {
     }
     is.close();
   }
+
+  public void testOpenDeleteConcurrently() throws IOException, Exception {
+    Path dir = wrap(createTempDir());
+    Path file = dir.resolve("thefile");
+    final CyclicBarrier barrier = new CyclicBarrier(2);
+    final AtomicBoolean stopped = new AtomicBoolean(false);
+    Thread t = new Thread() {
+      @Override
+      public void run() {
+        try {
+          barrier.await();
+        } catch (Exception ex) {
+          throw new RuntimeException(ex);
+        }
+        while (stopped.get() == false) {
+          try {
+            if (random().nextBoolean()) {
+              Files.delete(file);
+            } else if (random().nextBoolean()) {
+              Files.deleteIfExists(file);
+            } else {
+              Path target = file.resolveSibling("other");
+              Files.move(file, target);
+              Files.delete(target);
+            }
+          } catch (IOException ex) {
+            // continue
+          }
+        }
+      }
+    };
+    t.start();
+    barrier.await();
+    try {
+      final int iters = 10 + random().nextInt(100);
+      for (int i = 0; i < iters; i++) {
+        boolean opened = false;
+        try (OutputStream stream = Files.newOutputStream(file)) {
+          opened = true;
+          stream.write(0);
+          // just create
+        } catch (FileNotFoundException | NoSuchFileException ex) {
+          assertEquals("File handle leaked - file is closed but still regeistered", 0, ((WindowsFS) dir.getFileSystem().provider()).openFiles.size());
+          assertFalse("caught FNF on close", opened);
+        }
+        assertEquals("File handle leaked - file is closed but still regeistered", 0, ((WindowsFS) dir.getFileSystem().provider()).openFiles.size());
+        Files.deleteIfExists(file);
+      }
+    } finally {
+      stopped.set(true);
+      t.join();
+    }
+  }
 }