You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mi...@apache.org on 2011/12/20 18:52:01 UTC
svn commit: r1221368 - in /lucene/dev/trunk/lucene: ./
src/java/org/apache/lucene/store/
src/test-framework/java/org/apache/lucene/store/
src/test-framework/java/org/apache/lucene/util/
src/test/org/apache/lucene/index/
Author: mikemccand
Date: Tue Dec 20 17:52:00 2011
New Revision: 1221368
URL: http://svn.apache.org/viewvc?rev=1221368&view=rev
Log:
LUCENE-3658: fix bad asserts and concurrency issue in NRTCachingDir
Modified:
lucene/dev/trunk/lucene/CHANGES.txt
lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NRTCachingDirectory.java
lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/store/MockDirectoryWrapper.java
lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/util/LuceneTestCase.java
lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestCrash.java
lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestDoc.java
Modified: lucene/dev/trunk/lucene/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/CHANGES.txt?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/CHANGES.txt (original)
+++ lucene/dev/trunk/lucene/CHANGES.txt Tue Dec 20 17:52:00 2011
@@ -766,6 +766,10 @@ Bug fixes
double precision and to compute age to be how long ago the searcher
was replaced with a new searcher (Mike McCandless)
+* LUCENE-3658: Corrected potential concurrency issues with
+ NRTCachingDir, fixed createOutput to overwrite any previous file,
+ and removed invalid asserts (Robert Muir, Mike McCandless)
+
Optimizations
* LUCENE-3653: Improve concurrency in VirtualMethod and AttributeSource by
Modified: lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NRTCachingDirectory.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NRTCachingDirectory.java?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NRTCachingDirectory.java (original)
+++ lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NRTCachingDirectory.java Tue Dec 20 17:52:00 2011
@@ -167,7 +167,7 @@ public class NRTCachingDirectory extends
System.out.println("nrtdir.deleteFile name=" + name);
}
if (cache.fileExists(name)) {
- assert !delegate.fileExists(name);
+ assert !delegate.fileExists(name): "name=" + name;
cache.deleteFile(name);
} else {
delegate.deleteFile(name);
@@ -196,8 +196,18 @@ public class NRTCachingDirectory extends
if (VERBOSE) {
System.out.println(" to cache");
}
+ try {
+ delegate.deleteFile(name);
+ } catch (IOException ioe) {
+ // This is fine: file may not exist
+ }
return cache.createOutput(name, context);
} else {
+ try {
+ cache.deleteFile(name);
+ } catch (IOException ioe) {
+ // This is fine: file may not exist
+ }
return delegate.createOutput(name, context);
}
}
@@ -247,6 +257,11 @@ public class NRTCachingDirectory extends
* to the delegate and then closes the delegate. */
@Override
public void close() throws IOException {
+ // NOTE: technically we shouldn't have to do this, ie,
+ // IndexWriter should have sync'd all files, but we do
+ // it for defensive reasons... or in case the app is
+ // doing something custom (creating outputs directly w/o
+ // using IndexWriter):
for(String fileName : cache.listAll()) {
unCache(fileName);
}
@@ -262,29 +277,40 @@ public class NRTCachingDirectory extends
return !name.equals(IndexFileNames.SEGMENTS_GEN) && (merge == null || merge.estimatedMergeBytes <= maxMergeSizeBytes) && cache.sizeInBytes() <= maxCachedBytes;
}
+ private final Object uncacheLock = new Object();
+
private void unCache(String fileName) throws IOException {
- final IndexOutput out;
- IOContext context = IOContext.DEFAULT;
- synchronized(this) {
- if (!delegate.fileExists(fileName)) {
- assert cache.fileExists(fileName);
+ // Only let one thread uncache at a time; this only
+ // happens during commit() or close():
+ IndexOutput out = null;
+ IndexInput in = null;
+ try {
+ synchronized(uncacheLock) {
+ if (VERBOSE) {
+ System.out.println("nrtdir.unCache name=" + fileName);
+ }
+ if (!cache.fileExists(fileName)) {
+ // Another thread beat us...
+ return;
+ }
+ IOContext context = IOContext.DEFAULT;
+ if (delegate.fileExists(fileName)) {
+ throw new IOException("cannot uncache file=\"" + fileName + "\": it was separately also created in the delegate directory");
+ }
out = delegate.createOutput(fileName, context);
- } else {
- out = null;
- }
- }
- if (out != null) {
- IndexInput in = null;
- try {
in = cache.openInput(fileName, context);
in.copyBytes(out, in.length());
- } finally {
- IOUtils.close(in, out);
- }
- synchronized(this) {
- cache.deleteFile(fileName);
+
+ // Lock order: uncacheLock -> this
+ synchronized(this) {
+ // Must sync here because other sync methods have
+ // if (cache.fileExists(name)) { ... } else { ... }:
+ cache.deleteFile(fileName);
+ }
}
+ } finally {
+ IOUtils.close(in, out);
}
}
}
Modified: lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/store/MockDirectoryWrapper.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/store/MockDirectoryWrapper.java?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/store/MockDirectoryWrapper.java (original)
+++ lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/store/MockDirectoryWrapper.java Tue Dec 20 17:52:00 2011
@@ -220,9 +220,31 @@ public class MockDirectoryWrapper extend
} else if (damage == 2) {
action = "partially truncated";
// Partially Truncate the file:
- IndexOutput out = delegate.createOutput(name, LuceneTestCase.newIOContext(randomState));
- out.setLength(fileLength(name)/2);
+
+ // First, make temp file and copy only half this
+ // file over:
+ String tempFileName;
+ while (true) {
+ tempFileName = ""+randomState.nextInt();
+ if (!delegate.fileExists(tempFileName)) {
+ break;
+ }
+ }
+ final IndexOutput tempOut = delegate.createOutput(tempFileName, LuceneTestCase.newIOContext(randomState));
+ IndexInput in = delegate.openInput(name, LuceneTestCase.newIOContext(randomState));
+ tempOut.copyBytes(in, in.length()/2);
+ tempOut.close();
+ in.close();
+
+ // Delete original and copy bytes back:
+ deleteFile(name, true);
+
+ final IndexOutput out = delegate.createOutput(name, LuceneTestCase.newIOContext(randomState));
+ in = delegate.openInput(tempFileName, LuceneTestCase.newIOContext(randomState));
+ out.copyBytes(in, in.length());
out.close();
+ in.close();
+ deleteFile(tempFileName, true);
} else if (damage == 3) {
// The file survived intact:
action = "didn't change";
Modified: lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/util/LuceneTestCase.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/util/LuceneTestCase.java?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/util/LuceneTestCase.java (original)
+++ lucene/dev/trunk/lucene/src/test-framework/java/org/apache/lucene/util/LuceneTestCase.java Tue Dec 20 17:52:00 2011
@@ -1007,18 +1007,8 @@ public abstract class LuceneTestCase ext
* See {@link #newDirectory()} for more information.
*/
public static MockDirectoryWrapper newDirectory(Random r) throws IOException {
- return newDirectory(r, true);
- }
-
- /**
- * Returns a new Directory instance, using the specified random. You
- * can specify maybeWrap as to whether the directory might be also
- * wrapped by NRTCachingDirectory or FileSwitchDirectory
- * See {@link #newDirectory()} for more information.
- */
- public static MockDirectoryWrapper newDirectory(Random r, boolean maybeWrap) throws IOException {
Directory impl = newDirectoryImpl(r, TEST_DIRECTORY);
- MockDirectoryWrapper dir = new MockDirectoryWrapper(r, maybeWrap ? maybeNRTWrap(r, impl) : impl);
+ MockDirectoryWrapper dir = new MockDirectoryWrapper(r, maybeNRTWrap(r, impl));
stores.put(dir, Thread.currentThread().getStackTrace());
dir.setThrottling(TEST_THROTTLING);
return dir;
@@ -1030,7 +1020,7 @@ public abstract class LuceneTestCase ext
* information.
*/
public static MockDirectoryWrapper newDirectory(Directory d) throws IOException {
- return newDirectory(random, d, true);
+ return newDirectory(random, d);
}
/** Returns a new FSDirectory instance over the given file, which must be a folder. */
@@ -1040,11 +1030,6 @@ public abstract class LuceneTestCase ext
/** Returns a new FSDirectory instance over the given file, which must be a folder. */
public static MockDirectoryWrapper newFSDirectory(File f, LockFactory lf) throws IOException {
- return newFSDirectory(f, lf, true);
- }
-
- /** Returns a new FSDirectory instance over the given file, which must be a folder. */
- public static MockDirectoryWrapper newFSDirectory(File f, LockFactory lf, boolean maybeWrap) throws IOException {
String fsdirClass = TEST_DIRECTORY;
if (fsdirClass.equals("random")) {
fsdirClass = FS_DIRECTORIES[random.nextInt(FS_DIRECTORIES.length)];
@@ -1061,7 +1046,7 @@ public abstract class LuceneTestCase ext
}
Directory fsdir = newFSDirectoryImpl(clazz, f);
- MockDirectoryWrapper dir = new MockDirectoryWrapper(random, maybeWrap ? maybeNRTWrap(random, fsdir) : fsdir);
+ MockDirectoryWrapper dir = new MockDirectoryWrapper(random, maybeNRTWrap(random, fsdir));
if (lf != null) {
dir.setLockFactory(lf);
}
@@ -1078,12 +1063,12 @@ public abstract class LuceneTestCase ext
* with contents copied from the provided directory. See
* {@link #newDirectory()} for more information.
*/
- public static MockDirectoryWrapper newDirectory(Random r, Directory d, boolean maybeWrap) throws IOException {
+ public static MockDirectoryWrapper newDirectory(Random r, Directory d) throws IOException {
Directory impl = newDirectoryImpl(r, TEST_DIRECTORY);
for (String file : d.listAll()) {
d.copy(impl, file, file, newIOContext(r));
}
- MockDirectoryWrapper dir = new MockDirectoryWrapper(r, maybeWrap ? maybeNRTWrap(r, impl) : impl);
+ MockDirectoryWrapper dir = new MockDirectoryWrapper(r, maybeNRTWrap(r, impl));
stores.put(dir, Thread.currentThread().getStackTrace());
dir.setThrottling(TEST_THROTTLING);
return dir;
Modified: lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestCrash.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestCrash.java?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestCrash.java (original)
+++ lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestCrash.java Tue Dec 20 17:52:00 2011
@@ -30,9 +30,7 @@ import org.apache.lucene.document.TextFi
public class TestCrash extends LuceneTestCase {
private IndexWriter initIndex(Random random, boolean initialCommit) throws IOException {
- // note: we pass 'false' here so our crashing/deleting won't trigger assertions in NRTCachingDir
- // TODO: don't remember why this is ok... maybe we should check again that it really actually is.
- return initIndex(random, newDirectory(random, false), initialCommit);
+ return initIndex(random, newDirectory(random), initialCommit);
}
private IndexWriter initIndex(Random random, MockDirectoryWrapper dir, boolean initialCommit) throws IOException {
Modified: lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestDoc.java
URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestDoc.java?rev=1221368&r1=1221367&r2=1221368&view=diff
==============================================================================
--- lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestDoc.java (original)
+++ lucene/dev/trunk/lucene/src/test/org/apache/lucene/index/TestDoc.java Tue Dec 20 17:52:00 2011
@@ -114,8 +114,7 @@ public class TestDoc extends LuceneTestC
StringWriter sw = new StringWriter();
PrintWriter out = new PrintWriter(sw, true);
- // TODO: why does this test trigger NRTCachingDirectory's assert?
- Directory directory = newFSDirectory(indexDir, null, false);
+ Directory directory = newFSDirectory(indexDir, null);
IndexWriter writer = new IndexWriter(
directory,
newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random)).
@@ -143,14 +142,14 @@ public class TestDoc extends LuceneTestC
directory.close();
out.close();
sw.close();
+
String multiFileOutput = sw.getBuffer().toString();
//System.out.println(multiFileOutput);
sw = new StringWriter();
out = new PrintWriter(sw, true);
- // TODO: why does this test trigger NRTCachingDirectory's assert?
- directory = newFSDirectory(indexDir, null, false);
+ directory = newFSDirectory(indexDir, null);
writer = new IndexWriter(
directory,
newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random)).