fix sharing of native locks by diff dirs: LUCENE-678

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

CHANGES.txt

@@ -136,6 +136,10 @@ Bug fixes
 15. LUCENE-683: Fixed data corruption when reading lazy loaded fields.
     (Yonik Seeley)
 
+16. LUCENE-678: Fixed bug in NativeFSLockFactory which caused the same
+    lock to be shared between different directories.
+    (Michael McCandless via Yonik Seeley)
+
 Optimizations
 
   1. LUCENE-586: TermDocs.skipTo() is now more efficient for multi-segment

src/java/org/apache/lucene/store/NativeFSLockFactory.java

@@ -21,7 +21,7 @@ import java.nio.channels.FileLock;
 import java.io.File;
 import java.io.RandomAccessFile;
 import java.io.IOException;
-import java.util.Hashtable;
+import java.util.HashSet;
 import java.util.Random;
 
 /**
@@ -37,22 +37,23 @@ import java.util.Random;
  * {@link LockFactory} implementation.
  *
  * <p>The advantage of this lock factory over
- * SimpleFSLockFactory is that the locks should be
+ * {@link SimpleFSLockFactory} is that the locks should be
- * "correct", whereas SimpleFSLockFactory uses
+ * "correct", whereas {@link SimpleFSLockFactory} uses
- * java.io.File.createNewFile which has warnings about not
+ * java.io.File.createNewFile which
+ * <a target="_top" href="http://java.sun.com/j2se/1.4.2/docs/api/java/io/File.html#createNewFile()">has warnings</a> about not
  * using it for locking.  Furthermore, if the JVM crashes,
  * the OS will free any held locks, whereas
- * SimpleFSLockFactory will keep the locks held, requiring
+ * {@link SimpleFSLockFactory} will keep the locks held, requiring
  * manual removal before re-running Lucene.</p>
  *
- * <p>Note that, unlike SimpleFSLockFactory, the existence of
+ * <p>Note that, unlike {@link SimpleFSLockFactory}, the existence of
  * leftover lock files in the filesystem on exiting the JVM
  * is fine because the OS will free the locks held against
  * these files even though the files still remain.</p>
  *
  * <p>Native locks file names have the substring "-n-", which
  * you can use to differentiate them from lock files created
- * by SimpleFSLockFactory.</p>
+ * by {@link SimpleFSLockFactory}.</p>
  *
  * @see LockFactory
  */
@@ -71,35 +72,6 @@ public class NativeFSLockFactory extends LockFactory {
 
   private File lockDir;
 
-  /*
-   * The javadocs for FileChannel state that you should have
-   * a single instance of a FileChannel (per JVM) for all
-   * locking against a given file.  To do this, we ensure
-   * there's a single instance of NativeFSLockFactory per
-   * canonical lock directory, and then we always use a
-   * single lock instance (per lock name) if it's present.
-   */
-  private static Hashtable LOCK_FACTORIES = new Hashtable();
-
-  private Hashtable locks = new Hashtable();
-
-  protected NativeFSLockFactory(File lockDir)
-    throws IOException {
-
-    this.lockDir = lockDir;
-
-    // Ensure that lockDir exists and is a directory.
-    if (!lockDir.exists()) {
-      if (!lockDir.mkdirs())
-        throw new IOException("Cannot create directory: " +
-                              lockDir.getAbsolutePath());
-    } else if (!lockDir.isDirectory()) {
-      throw new IOException("Found regular file where directory expected: " + 
-                            lockDir.getAbsolutePath());
-    }
-    acquireTestLock();
-  }
-
   // Simple test to verify locking system is "working".  On
   // NFS, if it's misconfigured, you can hit long (35
   // second) timeouts which cause Lock.obtain to take far
@@ -122,61 +94,58 @@ public class NativeFSLockFactory extends LockFactory {
   }
 
   /**
-   * Returns a NativeFSLockFactory instance, storing lock
+   * Create a NativeFSLockFactory instance, storing lock
    * files into the default LOCK_DIR:
    * <code>org.apache.lucene.lockDir</code> system property,
-   * or (if that is null) then <code>java.io.tmpdir</code>.
+   * or (if that is null) then the
+   * <code>java.io.tmpdir</code> system property.
    */
-  public static NativeFSLockFactory getLockFactory() throws IOException {
+  public NativeFSLockFactory() throws IOException {
-    return getLockFactory(new File(LOCK_DIR));
+    this(new File(LOCK_DIR));
   }
 
   /**
-   * Returns a NativeFSLockFactory instance, storing lock
+   * Create a NativeFSLockFactory instance, storing lock
    * files into the specified lockDirName:
    *
    * @param lockDirName where lock files are created.
    */
-  public static NativeFSLockFactory getLockFactory(String lockDirName) throws IOException {
+  public NativeFSLockFactory(String lockDirName) throws IOException {
-    return getLockFactory(new File(lockDirName));
+    this(new File(lockDirName));
   }
 
   /**
-   * Returns a NativeFSLockFactory instance, storing lock
+   * Create a NativeFSLockFactory instance, storing lock
    * files into the specified lockDir:
    * 
    * @param lockDir where lock files are created.
    */
-  public static NativeFSLockFactory getLockFactory(File lockDir) throws IOException {
+  public NativeFSLockFactory(File lockDir) throws IOException {
-    lockDir = new File(lockDir.getCanonicalPath());
 
-    NativeFSLockFactory f;
+    this.lockDir = lockDir;
 
-    synchronized(LOCK_FACTORIES) {
+    // Ensure that lockDir exists and is a directory.
-      f = (NativeFSLockFactory) LOCK_FACTORIES.get(lockDir);
+    if (!lockDir.exists()) {
-      if (f == null) {
+      if (!lockDir.mkdirs())
-        f = new NativeFSLockFactory(lockDir);
+        throw new IOException("Cannot create directory: " +
-        LOCK_FACTORIES.put(lockDir, f);
+                              lockDir.getAbsolutePath());
-      }
+    } else if (!lockDir.isDirectory()) {
+      throw new IOException("Found regular file where directory expected: " + 
+                            lockDir.getAbsolutePath());
     }
 
-    return f;
+    acquireTestLock();
   }
 
   public synchronized Lock makeLock(String lockName) {
-    Lock l = (Lock) locks.get(lockName);
+    String fullName;
-    if (l == null) {
+    if (lockPrefix.equals("")) {
-      String fullName;
+      fullName = lockName;
-      if (lockPrefix.equals("")) {
+    } else {
-        fullName = lockName;
+      fullName = lockPrefix + "-n-" + lockName;
-      } else {
-        fullName = lockPrefix + "-n-" + lockName;
-      }
-
-      l = new NativeFSLock(lockDir, fullName);
-      locks.put(lockName, l);
     }
-    return l;
+
+    return new NativeFSLock(lockDir, fullName);
   }
 
   public void clearAllLocks() throws IOException {
@@ -209,6 +178,18 @@ class NativeFSLock extends Lock {
   private File path;
   private File lockDir;
 
+  /*
+   * The javadocs for FileChannel state that you should have
+   * a single instance of a FileChannel (per JVM) for all
+   * locking against a given file.  To ensure this, we have
+   * a single (static) HashSet that contains the file paths
+   * of all currently locked locks.  This protects against
+   * possible cases where different Directory instances in
+   * one JVM (each with their own NativeFSLockFactory
+   * instance) have set the same lock dir and lock prefix.
+   */
+  private static HashSet LOCK_HELD = new HashSet();
+
   public NativeFSLock(File lockDir, String lockFileName) {
     this.lockDir = lockDir;
     path = new File(lockDir, lockFileName);
@@ -217,7 +198,7 @@ class NativeFSLock extends Lock {
   public synchronized boolean obtain() throws IOException {
 
     if (isLocked()) {
-      // We are already locked:
+      // Our instance is already locked:
       return false;
     }
 
@@ -231,43 +212,86 @@ class NativeFSLock extends Lock {
                             lockDir.getAbsolutePath());
     }
 
-    f = new RandomAccessFile(path, "rw");
+    String canonicalPath = path.getCanonicalPath();
+
+    boolean markedHeld = false;
+
     try {
-      channel = f.getChannel();
+
-      try {
+      // Make sure nobody else in-process has this lock held
-        try {
+      // already, and, mark it held if not:
-          lock = channel.tryLock();
+
-        } catch (IOException e) {
+      synchronized(LOCK_HELD) {
-          // At least on OS X, we will sometimes get an
+        if (LOCK_HELD.contains(canonicalPath)) {
-          // intermittant "Permission Denied" IOException,
+          // Someone else in this JVM already has the lock:
-          // which seems to simply mean "you failed to get
+          return false;
-          // the lock".  But other IOExceptions could be
+        } else {
-          // "permanent" (eg, locking is not supported via
+          // This "reserves" the fact that we are the one
-          // the filesystem).  So, we record the failure
+          // thread trying to obtain this lock, so we own
-          // reason here; the timeout obtain (usually the
+          // the only instance of a channel against this
-          // one calling us) will use this as "root cause"
+          // file:
-          // if it fails to get the lock.
+          LOCK_HELD.add(canonicalPath);
-          failureReason = e;
+          markedHeld = true;
         }
-      } finally {
+      }
-        if (lock == null) {
+
+      try {
+        f = new RandomAccessFile(path, "rw");
+      } catch (IOException e) {
+        // On Windows, we can get intermittant "Access
+        // Denied" here.  So, we treat this as failure to
+        // acquire the lock, but, store the reason in case
+        // there is in fact a real error case.
+        failureReason = e;
+        f = null;
+      }
+
+      if (f != null) {
+        try {
+          channel = f.getChannel();
           try {
-            channel.close();
+            lock = channel.tryLock();
+          } catch (IOException e) {
+            // At least on OS X, we will sometimes get an
+            // intermittant "Permission Denied" IOException,
+            // which seems to simply mean "you failed to get
+            // the lock".  But other IOExceptions could be
+            // "permanent" (eg, locking is not supported via
+            // the filesystem).  So, we record the failure
+            // reason here; the timeout obtain (usually the
+            // one calling us) will use this as "root cause"
+            // if it fails to get the lock.
+            failureReason = e;
           } finally {
-            channel = null;
+            if (lock == null) {
+              try {
+                channel.close();
+              } finally {
+                channel = null;
+              }
+            }
+          }
+        } finally {
+          if (channel == null) {
+            try {
+              f.close();
+            } finally {
+              f = null;
+            }
           }
         }
       }
+
     } finally {
-      if (channel == null) {
+      if (markedHeld && !isLocked()) {
-        try {
+        synchronized(LOCK_HELD) {
-          f.close();
+          if (LOCK_HELD.contains(canonicalPath)) {
-        } finally {
+            LOCK_HELD.remove(canonicalPath);
-          f = null;
+          }
         }
       }
     }
-    return lock != null;
+    return isLocked();
   }
 
   public synchronized void release() {
@@ -285,6 +309,9 @@ class NativeFSLock extends Lock {
               f.close();
             } finally {
               f = null;
+              synchronized(LOCK_HELD) {
+                LOCK_HELD.remove(path.getCanonicalPath());
+              }
             }
           }
         }

src/test/org/apache/lucene/store/TestLockFactory.java

@@ -161,13 +161,26 @@ public class TestLockFactory extends TestCase {
                    SimpleFSLockFactory.class.isInstance(writer.getDirectory().getLockFactory()) ||
                    NativeFSLockFactory.class.isInstance(writer.getDirectory().getLockFactory()));
 
-        writer.close();
+        // Intentionally do not close the first writer here.
-
+        // The goal is to "simulate" a crashed writer and
+        // ensure the second writer, with create=true, is
+        // able to remove the lock files.  This works OK
+        // with SimpleFSLockFactory as the locking
+        // implementation.  Note, however, that this test
+        // will not work on WIN32 when we switch to
+        // NativeFSLockFactory as the default locking for
+        // FSDirectory because the second IndexWriter cannot
+        // remove those lock files since they are held open
+        // by the first writer.  This is because leaving the
+        // first IndexWriter open is not really a good way
+        // to simulate a crashed writer.
+        
         // Create a 2nd IndexWriter.  This should not fail:
         IndexWriter writer2 = null;
         try {
             writer2 = new IndexWriter(indexDirName, new WhitespaceAnalyzer(), true);
         } catch (IOException e) {
+            e.printStackTrace(System.out);
             fail("Should not have hit an IOException with two IndexWriters with create=true, on default SimpleFSLockFactory");
         }
 
@@ -175,6 +188,7 @@ public class TestLockFactory extends TestCase {
         if (writer2 != null) {
             writer2.close();
         }
+
         // Cleanup
         rmDir(indexDirName);
     }
@@ -274,7 +288,7 @@ public class TestLockFactory extends TestCase {
     // no unexpected exceptions are raised, but use
     // NativeFSLockFactory:
     public void testStressLocksNativeFSLockFactory() throws IOException {
-      _testStressLocks(NativeFSLockFactory.getLockFactory(), "index.TestLockFactory7");
+      _testStressLocks(new NativeFSLockFactory(), "index.TestLockFactory7");
     }
 
     public void _testStressLocks(LockFactory lockFactory, String indexDirName) throws IOException {
@@ -308,28 +322,59 @@ public class TestLockFactory extends TestCase {
     // Verify: NativeFSLockFactory works correctly
     public void testNativeFSLockFactory() throws IOException {
 
-      // Make sure we get identical instances:
+      NativeFSLockFactory f = new NativeFSLockFactory();
-      NativeFSLockFactory f = NativeFSLockFactory.getLockFactory();
+
-      NativeFSLockFactory f2 = NativeFSLockFactory.getLockFactory();
+      NativeFSLockFactory f2 = new NativeFSLockFactory();
-      assertTrue("got different NativeFSLockFactory instances for same directory",
-                 f == f2);
 
-      // Make sure we get identical locks:
       f.setLockPrefix("test");
       Lock l = f.makeLock("commit");
       Lock l2 = f.makeLock("commit");
-      assertTrue("got different Lock instances for same lock name",
-                 l == l2);
 
       assertTrue("failed to obtain lock", l.obtain());
-      assertTrue("succeeded in obtaining lock twice", !l.obtain());
+      assertTrue("succeeded in obtaining lock twice", !l2.obtain());
       l.release();
 
-      // Make sure we can obtain it again:
+      assertTrue("failed to obtain 2nd lock after first one was freed", l2.obtain());
+      l2.release();
+
+      // Make sure we can obtain first one again:
       assertTrue("failed to obtain lock", l.obtain());
       l.release();
     }
 
+    // Verify: NativeFSLockFactory assigns different lock
+    // prefixes to different directories:
+    public void testNativeFSLockFactoryPrefix() throws IOException {
+
+      // Make sure we get identical instances:
+      Directory dir1 = FSDirectory.getDirectory("TestLockFactory.8", true, new NativeFSLockFactory());
+      Directory dir2 = FSDirectory.getDirectory("TestLockFactory.9", true, new NativeFSLockFactory());
+
+      String prefix1 = dir1.getLockFactory().getLockPrefix();
+      String prefix2 = dir2.getLockFactory().getLockPrefix();
+
+      assertTrue("Native Lock Factories are incorrectly shared: dir1 and dir2 have same lock prefix '" + prefix1 + "'; they should be different",
+                 !prefix1.equals(prefix2));
+      rmDir("TestLockFactory.8");
+      rmDir("TestLockFactory.9");
+    }
+
+    // Verify: default LockFactory assigns different lock prefixes:
+    public void testDefaultFSLockFactoryPrefix() throws IOException {
+
+      // Make sure we get identical instances:
+      Directory dir1 = FSDirectory.getDirectory("TestLockFactory.10", true);
+      Directory dir2 = FSDirectory.getDirectory("TestLockFactory.11", true);
+
+      String prefix1 = dir1.getLockFactory().getLockPrefix();
+      String prefix2 = dir2.getLockFactory().getLockPrefix();
+
+      assertTrue("Default Lock Factories are incorrectly shared: dir1 and dir2 have same lock prefix '" + prefix1 + "'; they should be different",
+                 !prefix1.equals(prefix2));
+      rmDir("TestLockFactory.10");
+      rmDir("TestLockFactory.11");
+    }
+
     private class WriterThread extends Thread { 
         private Directory dir;
         private int numIteration;