You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-commits@lucene.apache.org by yo...@apache.org on 2006/10/19 23:03:25 UTC
svn commit: r465882 - in /lucene/java/trunk: CHANGES.txt
src/java/org/apache/lucene/store/NativeFSLockFactory.java
src/test/org/apache/lucene/store/TestLockFactory.java
Author: yonik
Date: Thu Oct 19 14:03:22 2006
New Revision: 465882
URL: http://svn.apache.org/viewvc?view=rev&rev=465882
Log:
fix sharing of native locks by diff dirs: LUCENE-678
Modified:
lucene/java/trunk/CHANGES.txt
lucene/java/trunk/src/java/org/apache/lucene/store/NativeFSLockFactory.java
lucene/java/trunk/src/test/org/apache/lucene/store/TestLockFactory.java
Modified: lucene/java/trunk/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/java/trunk/CHANGES.txt?view=diff&rev=465882&r1=465881&r2=465882
==============================================================================
--- lucene/java/trunk/CHANGES.txt (original)
+++ lucene/java/trunk/CHANGES.txt Thu Oct 19 14:03:22 2006
@@ -136,6 +136,10 @@
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
Modified: lucene/java/trunk/src/java/org/apache/lucene/store/NativeFSLockFactory.java
URL: http://svn.apache.org/viewvc/lucene/java/trunk/src/java/org/apache/lucene/store/NativeFSLockFactory.java?view=diff&rev=465882&r1=465881&r2=465882
==============================================================================
--- lucene/java/trunk/src/java/org/apache/lucene/store/NativeFSLockFactory.java (original)
+++ lucene/java/trunk/src/java/org/apache/lucene/store/NativeFSLockFactory.java Thu Oct 19 14:03:22 2006
@@ -21,7 +21,7 @@
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 @@
* {@link LockFactory} implementation.
*
* <p>The advantage of this lock factory over
- * SimpleFSLockFactory is that the locks should be
- * "correct", whereas SimpleFSLockFactory uses
- * java.io.File.createNewFile which has warnings about not
+ * {@link SimpleFSLockFactory} is that the locks should be
+ * "correct", whereas {@link SimpleFSLockFactory} uses
+ * 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 @@
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 @@
}
/**
- * 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 {
- return getLockFactory(new File(LOCK_DIR));
+ public NativeFSLockFactory() throws IOException {
+ 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 {
- return getLockFactory(new File(lockDirName));
+ public NativeFSLockFactory(String lockDirName) throws IOException {
+ 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 {
- lockDir = new File(lockDir.getCanonicalPath());
+ public NativeFSLockFactory(File lockDir) throws IOException {
- NativeFSLockFactory f;
+ this.lockDir = lockDir;
- synchronized(LOCK_FACTORIES) {
- f = (NativeFSLockFactory) LOCK_FACTORIES.get(lockDir);
- if (f == null) {
- f = new NativeFSLockFactory(lockDir);
- LOCK_FACTORIES.put(lockDir, f);
- }
+ // 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());
}
- return f;
+ acquireTestLock();
}
public synchronized Lock makeLock(String lockName) {
- Lock l = (Lock) locks.get(lockName);
- if (l == null) {
- String fullName;
- if (lockPrefix.equals("")) {
- fullName = lockName;
- } else {
- fullName = lockPrefix + "-n-" + lockName;
- }
-
- l = new NativeFSLock(lockDir, fullName);
- locks.put(lockName, l);
+ String fullName;
+ if (lockPrefix.equals("")) {
+ fullName = lockName;
+ } else {
+ fullName = lockPrefix + "-n-" + lockName;
}
- return l;
+
+ return new NativeFSLock(lockDir, fullName);
}
public void clearAllLocks() throws IOException {
@@ -209,6 +178,18 @@
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 @@
public synchronized boolean obtain() throws IOException {
if (isLocked()) {
- // We are already locked:
+ // Our instance is already locked:
return false;
}
@@ -231,43 +212,86 @@
lockDir.getAbsolutePath());
}
- f = new RandomAccessFile(path, "rw");
+ String canonicalPath = path.getCanonicalPath();
+
+ boolean markedHeld = false;
+
try {
- channel = f.getChannel();
+
+ // Make sure nobody else in-process has this lock held
+ // already, and, mark it held if not:
+
+ synchronized(LOCK_HELD) {
+ if (LOCK_HELD.contains(canonicalPath)) {
+ // Someone else in this JVM already has the lock:
+ return false;
+ } else {
+ // This "reserves" the fact that we are the one
+ // thread trying to obtain this lock, so we own
+ // the only instance of a channel against this
+ // file:
+ LOCK_HELD.add(canonicalPath);
+ markedHeld = true;
+ }
+ }
+
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 {
- 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 {
- if (lock == null) {
+ 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) {
- try {
- f.close();
- } finally {
- f = null;
+ if (markedHeld && !isLocked()) {
+ synchronized(LOCK_HELD) {
+ if (LOCK_HELD.contains(canonicalPath)) {
+ LOCK_HELD.remove(canonicalPath);
+ }
}
}
}
- return lock != null;
+ return isLocked();
}
public synchronized void release() {
@@ -285,6 +309,9 @@
f.close();
} finally {
f = null;
+ synchronized(LOCK_HELD) {
+ LOCK_HELD.remove(path.getCanonicalPath());
+ }
}
}
}
Modified: lucene/java/trunk/src/test/org/apache/lucene/store/TestLockFactory.java
URL: http://svn.apache.org/viewvc/lucene/java/trunk/src/test/org/apache/lucene/store/TestLockFactory.java?view=diff&rev=465882&r1=465881&r2=465882
==============================================================================
--- lucene/java/trunk/src/test/org/apache/lucene/store/TestLockFactory.java (original)
+++ lucene/java/trunk/src/test/org/apache/lucene/store/TestLockFactory.java Thu Oct 19 14:03:22 2006
@@ -161,13 +161,26 @@
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 @@
if (writer2 != null) {
writer2.close();
}
+
// Cleanup
rmDir(indexDirName);
}
@@ -274,7 +288,7 @@
// 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,26 +322,57 @@
// Verify: NativeFSLockFactory works correctly
public void testNativeFSLockFactory() throws IOException {
- // Make sure we get identical instances:
- NativeFSLockFactory f = NativeFSLockFactory.getLockFactory();
- NativeFSLockFactory f2 = NativeFSLockFactory.getLockFactory();
- assertTrue("got different NativeFSLockFactory instances for same directory",
- f == f2);
+ NativeFSLockFactory f = new NativeFSLockFactory();
+
+ NativeFSLockFactory f2 = new NativeFSLockFactory();
- // 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 {