LUCENE-3439: catch more cases where IW/IR is used after close; fix sneaky bug where IW can delete files after it was closed
git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/branches/branch_3x@1173741 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index ba3ec5d..66f716a 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -43,6 +43,10 @@
directly into the FieldCache arrays during sorting, leading to concurrency
issues. (Uwe Schindler, Doron Cohen, Mike McCandless)
+* LUCENE-3439: Closing an NRT reader after the writer was closed was
+ incorrectly invoking the DeletionPolicy and (then possibly deleting
+ files) on the closed IndexWriter (Robert Muir, Mike McCandless)
+
New Features
Optimizations
diff --git a/lucene/backwards/src/test/org/apache/lucene/index/TestIndexReader.java b/lucene/backwards/src/test/org/apache/lucene/index/TestIndexReader.java
index d2d3050..fb89d00 100644
--- a/lucene/backwards/src/test/org/apache/lucene/index/TestIndexReader.java
+++ b/lucene/backwards/src/test/org/apache/lucene/index/TestIndexReader.java
@@ -73,18 +73,16 @@
IndexReader r = IndexReader.open(d, false);
r.deleteDocument(5);
r.flush(commitUserData);
+ IndexCommit c = r.getIndexCommit();
r.close();
SegmentInfos sis = new SegmentInfos();
sis.read(d);
IndexReader r2 = IndexReader.open(d, false);
- IndexCommit c = r.getIndexCommit();
assertEquals(c.getUserData(), commitUserData);
assertEquals(sis.getCurrentSegmentFileName(), c.getSegmentsFileName());
- assertTrue(c.equals(r.getIndexCommit()));
-
// Change the index
writer = new IndexWriter(d, newIndexWriterConfig(TEST_VERSION_CURRENT,
new MockAnalyzer(random)).setOpenMode(
diff --git a/lucene/src/java/org/apache/lucene/index/DirectoryReader.java b/lucene/src/java/org/apache/lucene/index/DirectoryReader.java
index 44b3903..947fd1b 100644
--- a/lucene/src/java/org/apache/lucene/index/DirectoryReader.java
+++ b/lucene/src/java/org/apache/lucene/index/DirectoryReader.java
@@ -359,6 +359,7 @@
@Override
public final synchronized IndexReader clone(boolean openReadOnly) throws CorruptIndexException, IOException {
+ // doReopen calls ensureOpen
DirectoryReader newReader = doReopen((SegmentInfos) segmentInfos.clone(), true, openReadOnly);
if (this != newReader) {
@@ -573,7 +574,7 @@
@Override
public boolean hasDeletions() {
- // Don't call ensureOpen() here (it could affect performance)
+ ensureOpen();
return hasDeletions;
}
@@ -901,7 +902,7 @@
if (writer != null) {
// Since we just closed, writer may now be able to
// delete unused files:
- writer.deleteUnusedFiles();
+ writer.deletePendingFiles();
}
// throw the first exception
@@ -940,6 +941,7 @@
@Override
public int getTermInfosIndexDivisor() {
+ ensureOpen();
return termInfosIndexDivisor;
}
@@ -950,6 +952,7 @@
*/
@Override
public IndexCommit getIndexCommit() throws IOException {
+ ensureOpen();
return new ReaderCommit(segmentInfos, directory);
}
diff --git a/lucene/src/java/org/apache/lucene/index/FilterIndexReader.java b/lucene/src/java/org/apache/lucene/index/FilterIndexReader.java
index 5717091..c989729 100644
--- a/lucene/src/java/org/apache/lucene/index/FilterIndexReader.java
+++ b/lucene/src/java/org/apache/lucene/index/FilterIndexReader.java
@@ -114,6 +114,7 @@
@Override
public Directory directory() {
+ ensureOpen();
return in.directory();
}
@@ -136,7 +137,6 @@
public void getTermFreqVector(int docNumber, String field, TermVectorMapper mapper) throws IOException {
ensureOpen();
in.getTermFreqVector(docNumber, field, mapper);
-
}
@Override
@@ -171,7 +171,7 @@
@Override
public boolean hasDeletions() {
- // Don't call ensureOpen() here (it could affect performance)
+ ensureOpen();
return in.hasDeletions();
}
@@ -241,14 +241,15 @@
protected void doDelete(int n) throws CorruptIndexException, IOException { in.deleteDocument(n); }
@Override
- protected void doCommit(Map<String,String> commitUserData) throws IOException { in.commit(commitUserData); }
+ protected void doCommit(Map<String,String> commitUserData) throws IOException {
+ in.commit(commitUserData);
+ }
@Override
protected void doClose() throws IOException {
in.close();
}
-
@Override
public Collection<String> getFieldNames(IndexReader.FieldOption fieldNames) {
ensureOpen();
diff --git a/lucene/src/java/org/apache/lucene/index/IndexReader.java b/lucene/src/java/org/apache/lucene/index/IndexReader.java
index ca689bf..4533303 100644
--- a/lucene/src/java/org/apache/lucene/index/IndexReader.java
+++ b/lucene/src/java/org/apache/lucene/index/IndexReader.java
@@ -109,6 +109,7 @@
*
* @lucene.experimental */
public void addReaderFinishedListener(ReaderFinishedListener listener) {
+ ensureOpen();
readerFinishedListeners.add(listener);
}
@@ -116,6 +117,7 @@
*
* @lucene.experimental */
public void removeReaderFinishedListener(ReaderFinishedListener listener) {
+ ensureOpen();
readerFinishedListeners.remove(listener);
}
@@ -164,7 +166,7 @@
TERMVECTOR_WITH_POSITION_OFFSET,
}
- private boolean closed;
+ private volatile boolean closed;
protected boolean hasChanges;
private final AtomicInteger refCount = new AtomicInteger();
@@ -228,7 +230,8 @@
*/
public void decRef() throws IOException {
ensureOpen();
- if (refCount.getAndDecrement() == 1) {
+ final int rc = refCount.getAndDecrement();
+ if (rc == 1) {
boolean success = false;
try {
commit();
@@ -241,6 +244,8 @@
}
}
readerFinished();
+ } else if (rc <= 0) {
+ throw new IllegalStateException("too many decRef calls: refCount was " + rc + " before decrement");
}
}
@@ -1186,6 +1191,7 @@
*/
public final synchronized void commit(Map<String, String> commitUserData) throws IOException {
if (hasChanges) {
+ // Don't call ensureOpen since we commit() on close
doCommit(commitUserData);
}
hasChanges = false;
@@ -1345,11 +1351,14 @@
* corruption for other readers (like DirectoryReader obtained
* through {@link #open}. Use the parent reader directly. */
public IndexReader[] getSequentialSubReaders() {
+ ensureOpen();
return null;
}
/** Expert */
public Object getCoreCacheKey() {
+ // Don't can ensureOpen since FC calls this (to evict)
+ // on close
return this;
}
diff --git a/lucene/src/java/org/apache/lucene/index/IndexWriter.java b/lucene/src/java/org/apache/lucene/index/IndexWriter.java
index 91eb749..ad40b4d 100644
--- a/lucene/src/java/org/apache/lucene/index/IndexWriter.java
+++ b/lucene/src/java/org/apache/lucene/index/IndexWriter.java
@@ -292,8 +292,8 @@
private Lock writeLock;
- private boolean closed;
- private boolean closing;
+ private volatile boolean closed;
+ private volatile boolean closing;
// Holds all SegmentInfo instances currently involved in
// merges
@@ -755,6 +755,7 @@
* delCount is returned.
*/
public int numDeletedDocs(SegmentInfo info) throws IOException {
+ ensureOpen(false);
SegmentReader reader = readerPool.getIfExists(info);
try {
if (reader != null) {
@@ -1268,6 +1269,7 @@
* IndexWriterConfig} for details.
*/
public IndexWriterConfig getConfig() {
+ ensureOpen(false);
return config;
}
@@ -1930,6 +1932,7 @@
* not counting deletions.
* @see #numDocs */
public synchronized int maxDoc() {
+ ensureOpen();
int count;
if (docWriter != null)
count = docWriter.getNumDocs();
@@ -1947,6 +1950,7 @@
* counted you should call {@link #commit()} first.
* @see #numDocs */
public synchronized int numDocs() throws IOException {
+ ensureOpen();
int count;
if (docWriter != null)
count = docWriter.getNumDocs();
@@ -2728,6 +2732,7 @@
}
private final void maybeMerge(int maxNumSegmentsOptimize, boolean optimize) throws CorruptIndexException, IOException {
+ ensureOpen(false);
updatePendingMerges(maxNumSegmentsOptimize, optimize);
mergeScheduler.merge(this);
}
@@ -2911,6 +2916,7 @@
* {@link MergePolicy.MergeAbortedException}s.
*/
public synchronized void deleteAll() throws IOException {
+ ensureOpen();
try {
// Abort any running merges
@@ -2997,6 +3003,7 @@
* will have completed once this method completes.</p>
*/
public synchronized void waitForMerges() {
+ ensureOpen(false);
if (infoStream != null) {
message("waitForMerges");
}
@@ -3378,6 +3385,7 @@
*/
public final void prepareCommit(Map<String, String> commitUserData)
throws CorruptIndexException, IOException {
+ ensureOpen(false);
if (hitOOM) {
throw new IllegalStateException(
@@ -4790,10 +4798,16 @@
* be deleted the next time commit() is called.
*/
public synchronized void deleteUnusedFiles() throws IOException {
+ ensureOpen(false);
deleter.deletePendingFiles();
deleter.revisitPolicy();
}
+ // Called by DirectoryReader.doClose
+ synchronized void deletePendingFiles() throws IOException {
+ deleter.deletePendingFiles();
+ }
+
/**
* Sets the {@link PayloadProcessorProvider} to use when merging payloads.
* Note that the given <code>pcp</code> will be invoked for every segment that
@@ -4813,6 +4827,7 @@
* {@link #optimize}, you can call {@link #waitForMerges()} before.
*/
public void setPayloadProcessorProvider(PayloadProcessorProvider pcp) {
+ ensureOpen();
payloadProcessorProvider = pcp;
}
@@ -4821,6 +4836,7 @@
* merges to process payloads.
*/
public PayloadProcessorProvider getPayloadProcessorProvider() {
+ ensureOpen();
return payloadProcessorProvider;
}
diff --git a/lucene/src/java/org/apache/lucene/index/MultiReader.java b/lucene/src/java/org/apache/lucene/index/MultiReader.java
index 3a065bb..1a4b602 100644
--- a/lucene/src/java/org/apache/lucene/index/MultiReader.java
+++ b/lucene/src/java/org/apache/lucene/index/MultiReader.java
@@ -220,6 +220,7 @@
@Override
public boolean isOptimized() {
+ ensureOpen();
return false;
}
@@ -260,7 +261,7 @@
@Override
public boolean hasDeletions() {
- // Don't call ensureOpen() here (it could affect performance)
+ ensureOpen();
return hasDeletions;
}
@@ -431,6 +432,7 @@
*/
@Override
public boolean isCurrent() throws CorruptIndexException, IOException {
+ ensureOpen();
for (int i = 0; i < subReaders.length; i++) {
if (!subReaders[i].isCurrent()) {
return false;
diff --git a/lucene/src/java/org/apache/lucene/index/ParallelReader.java b/lucene/src/java/org/apache/lucene/index/ParallelReader.java
index f2c435a..4334d3b 100644
--- a/lucene/src/java/org/apache/lucene/index/ParallelReader.java
+++ b/lucene/src/java/org/apache/lucene/index/ParallelReader.java
@@ -141,6 +141,7 @@
@Override
public synchronized Object clone() {
+ // doReopen calls ensureOpen
try {
return doReopen(true);
} catch (Exception ex) {
@@ -169,6 +170,7 @@
*/
@Override
public synchronized IndexReader reopen() throws CorruptIndexException, IOException {
+ // doReopen calls ensureOpen
return doReopen(false);
}
@@ -251,7 +253,7 @@
@Override
public boolean hasDeletions() {
- // Don't call ensureOpen() here (it could affect performance)
+ ensureOpen();
return hasDeletions;
}
@@ -434,6 +436,7 @@
*/
@Override
public boolean isCurrent() throws CorruptIndexException, IOException {
+ ensureOpen();
for (final IndexReader reader : readers) {
if (!reader.isCurrent()) {
return false;
@@ -449,6 +452,7 @@
*/
@Override
public boolean isOptimized() {
+ ensureOpen();
for (final IndexReader reader : readers) {
if (!reader.isOptimized()) {
return false;
@@ -574,7 +578,6 @@
if (termEnum!=null)
termEnum.close();
}
-
}
// wrap a TermDocs in order to support seek(Term)
diff --git a/lucene/src/java/org/apache/lucene/index/SegmentReader.java b/lucene/src/java/org/apache/lucene/index/SegmentReader.java
index 527ac99..6cc9a2c 100644
--- a/lucene/src/java/org/apache/lucene/index/SegmentReader.java
+++ b/lucene/src/java/org/apache/lucene/index/SegmentReader.java
@@ -183,6 +183,7 @@
* @return New BitVector
*/
protected BitVector cloneDeletedDocs(BitVector bv) {
+ ensureOpen();
return (BitVector)bv.clone();
}
@@ -213,6 +214,7 @@
}
synchronized SegmentReader reopenSegment(SegmentInfo si, boolean doClone, boolean openReadOnly) throws CorruptIndexException, IOException {
+ ensureOpen();
boolean deletionsUpToDate = (this.si.hasDeletions() == si.hasDeletions())
&& (!si.hasDeletions() || this.si.getDelFileName().equals(si.getDelFileName()));
boolean normsUpToDate = true;
diff --git a/lucene/src/java/org/apache/lucene/search/TermRangeQuery.java b/lucene/src/java/org/apache/lucene/search/TermRangeQuery.java
index 383883d..1fca930 100644
--- a/lucene/src/java/org/apache/lucene/search/TermRangeQuery.java
+++ b/lucene/src/java/org/apache/lucene/search/TermRangeQuery.java
@@ -103,7 +103,7 @@
* <code>upperTerm</code>.
*/
public TermRangeQuery(String field, String lowerTerm, String upperTerm, boolean includeLower, boolean includeUpper,
- Collator collator) {
+ Collator collator) {
this.field = field;
this.lowerTerm = lowerTerm;
this.upperTerm = upperTerm;
diff --git a/lucene/src/test/org/apache/lucene/index/TestIndexReader.java b/lucene/src/test/org/apache/lucene/index/TestIndexReader.java
index d2d3050..fb89d00 100644
--- a/lucene/src/test/org/apache/lucene/index/TestIndexReader.java
+++ b/lucene/src/test/org/apache/lucene/index/TestIndexReader.java
@@ -73,18 +73,16 @@
IndexReader r = IndexReader.open(d, false);
r.deleteDocument(5);
r.flush(commitUserData);
+ IndexCommit c = r.getIndexCommit();
r.close();
SegmentInfos sis = new SegmentInfos();
sis.read(d);
IndexReader r2 = IndexReader.open(d, false);
- IndexCommit c = r.getIndexCommit();
assertEquals(c.getUserData(), commitUserData);
assertEquals(sis.getCurrentSegmentFileName(), c.getSegmentsFileName());
- assertTrue(c.equals(r.getIndexCommit()));
-
// Change the index
writer = new IndexWriter(d, newIndexWriterConfig(TEST_VERSION_CURRENT,
new MockAnalyzer(random)).setOpenMode(
diff --git a/lucene/src/test/org/apache/lucene/index/TestReaderClosed.java b/lucene/src/test/org/apache/lucene/index/TestReaderClosed.java
new file mode 100644
index 0000000..7123d4b
--- /dev/null
+++ b/lucene/src/test/org/apache/lucene/index/TestReaderClosed.java
@@ -0,0 +1,76 @@
+package org.apache.lucene.index;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import org.apache.lucene.analysis.MockAnalyzer;
+import org.apache.lucene.analysis.MockTokenizer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.TermRangeQuery;
+import org.apache.lucene.store.AlreadyClosedException;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util._TestUtil;
+
+public class TestReaderClosed extends LuceneTestCase {
+ private IndexSearcher searcher;
+ private IndexReader reader;
+ private Directory dir;
+
+ @Override
+ public void setUp() throws Exception {
+ super.setUp();
+ dir = newDirectory();
+ RandomIndexWriter writer = new RandomIndexWriter(random, dir,
+ newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random, MockTokenizer.KEYWORD, false))
+ .setMaxBufferedDocs(_TestUtil.nextInt(random, 50, 1000)));
+
+ Document doc = new Document();
+ Field field = newField("field", "", Field.Store.NO, Field.Index.NOT_ANALYZED);
+ doc.add(field);
+
+ // we generate aweful prefixes: good for testing.
+ // but for preflex codec, the test can be very slow, so use less iterations.
+ int num = atLeast(10);
+ for (int i = 0; i < num; i++) {
+ field.setValue(_TestUtil.randomUnicodeString(random, 10));
+ writer.addDocument(doc);
+ }
+ reader = writer.getReader();
+ searcher = newSearcher(reader);
+ writer.close();
+ }
+
+ public void test() throws Exception {
+ TermRangeQuery query = new TermRangeQuery("field", "a", "z", true, true);
+ searcher.search(query, 5);
+ searcher.close();
+ reader.close();
+ try {
+ searcher.search(query, 5);
+ } catch (AlreadyClosedException ace) {
+ // expected
+ }
+ }
+
+ public void tearDown() throws Exception {
+ dir.close();
+ super.tearDown();
+ }
+}
