Index: src/test/org/apache/lucene/index/TestIndexReaderClone.java =================================================================== --- src/test/org/apache/lucene/index/TestIndexReaderClone.java (revision 748341) +++ src/test/org/apache/lucene/index/TestIndexReaderClone.java (working copy) @@ -423,4 +423,21 @@ } dir1.close(); } + + public void testLucene1516Bug() throws Exception { + final Directory dir1 = new MockRAMDirectory(); + TestIndexReaderReopen.createIndex(dir1, false); + IndexReader r1 = IndexReader.open(dir1); + r1.incRef(); + IndexReader r2 = (IndexReader) r1.clone(false); + r1.deleteDocument(5); + r1.decRef(); + + r1.incRef(); + + r2.close(); + r1.decRef(); + r1.close(); + dir1.close(); + } } Index: src/test/org/apache/lucene/index/TestIndexReaderReopen.java =================================================================== --- src/test/org/apache/lucene/index/TestIndexReaderReopen.java (revision 748341) +++ src/test/org/apache/lucene/index/TestIndexReaderReopen.java (working copy) @@ -364,9 +364,11 @@ assertEquals(subReaders0.length, subReaders1.length); for (int i = 0; i < subReaders0.length; i++) { - assertRefCountEquals(2, subReaders0[i]); if (subReaders0[i] != subReaders1[i]) { + assertRefCountEquals(1, subReaders0[i]); assertRefCountEquals(1, subReaders1[i]); + } else { + assertRefCountEquals(2, subReaders0[i]); } } @@ -390,10 +392,10 @@ } else { assertRefCountEquals(1, subReaders2[i]); if (subReaders0[i] == subReaders1[i]) { - assertRefCountEquals(3, subReaders2[i]); + assertRefCountEquals(2, subReaders2[i]); assertRefCountEquals(2, subReaders0[i]); } else { - assertRefCountEquals(3, subReaders0[i]); + assertRefCountEquals(1, subReaders0[i]); assertRefCountEquals(1, subReaders1[i]); } } @@ -463,7 +465,7 @@ modifyIndex(0, dir1); IndexReader reader2 = reader1.reopen(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(2 + mode, reader1); if (mode == 1) { initReader2.close(); @@ -471,31 +473,31 @@ modifyIndex(1, dir1); IndexReader reader3 = reader2.reopen(); - assertRefCountEquals(4 + mode, reader1); + assertRefCountEquals(2 + mode, reader1); assertRefCountEquals(1, reader2); multiReader1.close(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(1 + mode, reader1); multiReader1.close(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(1 + mode, reader1); if (mode == 1) { initReader2.close(); } reader1.close(); - assertRefCountEquals(3, reader1); + assertRefCountEquals(1, reader1); multiReader2.close(); - assertRefCountEquals(2, reader1); + assertRefCountEquals(0, reader1); multiReader2.close(); - assertRefCountEquals(2, reader1); + assertRefCountEquals(0, reader1); reader3.close(); - assertRefCountEquals(1, reader1); - assertReaderOpen(reader1); + assertRefCountEquals(0, reader1); + assertReaderClosed(reader1, true, false); reader2.close(); assertRefCountEquals(0, reader1); @@ -537,7 +539,7 @@ modifyIndex(0, dir1); modifyIndex(0, dir2); IndexReader reader2 = reader1.reopen(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(2 + mode, reader1); if (mode == 1) { initReader2.close(); @@ -545,31 +547,31 @@ modifyIndex(4, dir1); IndexReader reader3 = reader2.reopen(); - assertRefCountEquals(4 + mode, reader1); + assertRefCountEquals(2 + mode, reader1); assertRefCountEquals(1, reader2); parallelReader1.close(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(1 + mode, reader1); parallelReader1.close(); - assertRefCountEquals(3 + mode, reader1); + assertRefCountEquals(1 + mode, reader1); if (mode == 1) { initReader2.close(); } reader1.close(); - assertRefCountEquals(3, reader1); + assertRefCountEquals(1, reader1); parallelReader2.close(); - assertRefCountEquals(2, reader1); + assertRefCountEquals(0, reader1); parallelReader2.close(); - assertRefCountEquals(2, reader1); + assertRefCountEquals(0, reader1); reader3.close(); - assertRefCountEquals(1, reader1); - assertReaderOpen(reader1); + assertRefCountEquals(0, reader1); + assertReaderClosed(reader1, true, false); reader2.close(); assertRefCountEquals(0, reader1); @@ -617,16 +619,16 @@ // Now reader2-reader5 references reader1. reader1 and reader2 // share the same norms. reader3, reader4, reader5 also share norms. - assertRefCountEquals(5, reader1); + assertRefCountEquals(1, reader1); assertFalse(reader1.normsClosed()); reader1.close(); - assertRefCountEquals(4, reader1); + assertRefCountEquals(0, reader1); assertFalse(reader1.normsClosed()); reader2.close(); - assertRefCountEquals(3, reader1); + assertRefCountEquals(0, reader1); // now the norms for field1 and field2 should be closed assertTrue(reader1.normsClosed("field1")); @@ -637,10 +639,10 @@ assertFalse(reader1.normsClosed("field4")); reader3.close(); - assertRefCountEquals(2, reader1); + assertRefCountEquals(0, reader1); assertFalse(reader3.normsClosed()); reader5.close(); - assertRefCountEquals(1, reader1); + assertRefCountEquals(0, reader1); assertFalse(reader3.normsClosed()); reader4.close(); assertRefCountEquals(0, reader1); @@ -1140,4 +1142,87 @@ // expected } } + + public void testCloseOrig() throws Throwable { + Directory dir = new MockRAMDirectory(); + createIndex(dir, false); + IndexReader r1 = IndexReader.open(dir); + IndexReader r2 = IndexReader.open(dir); + r2.deleteDocument(0); + r2.close(); + + IndexReader r3 = r1.reopen(); + assertTrue(r1 != r3); + r1.close(); + try { + r1.document(2); + fail("did not hit exception"); + } catch (AlreadyClosedException ace) { + // expected + } + r3.close(); + dir.close(); + } + + public void testDeletes() throws Throwable { + Directory dir = new MockRAMDirectory(); + createIndex(dir, false); + // Get delete bitVector + modifyIndex(0, dir); + IndexReader r1 = IndexReader.open(dir); + + // Add doc: + modifyIndex(5, dir); + + IndexReader r2 = r1.reopen(); + assertTrue(r1 != r2); + + IndexReader[] rs2 = r2.getSequentialSubReaders(); + + SegmentReader sr1 = (SegmentReader) r1; + SegmentReader sr2 = (SegmentReader) rs2[0]; + + // At this point they share the same BitVector + assertTrue(sr1.deletedDocs==sr2.deletedDocs); + + r2.deleteDocument(0); + + // r1 should not see the delete + assertFalse(r1.isDeleted(0)); + + // Now r2 should have made a private copy of deleted docs: + assertTrue(sr1.deletedDocs!=sr2.deletedDocs); + + r1.close(); + r2.close(); + dir.close(); + } + + public void testDeletes2() throws Throwable { + Directory dir = new MockRAMDirectory(); + createIndex(dir, false); + // Get delete bitVector + modifyIndex(0, dir); + IndexReader r1 = IndexReader.open(dir); + + // Add doc: + modifyIndex(5, dir); + + IndexReader r2 = r1.reopen(); + assertTrue(r1 != r2); + + IndexReader[] rs2 = r2.getSequentialSubReaders(); + + SegmentReader sr1 = (SegmentReader) r1; + SegmentReader sr2 = (SegmentReader) rs2[0]; + + // At this point they share the same BitVector + assertTrue(sr1.deletedDocs==sr2.deletedDocs); + r1.close(); + + r2.deleteDocument(0); + assertTrue(sr1.deletedDocs==sr2.deletedDocs); + r2.close(); + dir.close(); + } } Index: src/java/org/apache/lucene/index/SegmentReader.java =================================================================== --- src/java/org/apache/lucene/index/SegmentReader.java (revision 748341) +++ src/java/org/apache/lucene/index/SegmentReader.java (working copy) @@ -71,14 +71,18 @@ private IndexInput singleNormStream; private Ref singleNormRef; + // Counts how many other reader share the core objects + // (freqStream, proxStream, tis, etc.) of this reader; + // when coreRef drops to 0, these core objects may be + // closed. A given insance of SegmentReader may be + // closed, even those it shares core objects with other + // SegmentReaders: + private Ref coreRef = new Ref(); + // Compound File Reader when based on a compound file segment CompoundFileReader cfsReader = null; CompoundFileReader storeCFSReader = null; - // indicates the SegmentReader with which the resources are being shared, - // in case this is a re-opened reader - private SegmentReader referencedSegmentReader = null; - /** * Sets the initial value */ @@ -331,37 +335,6 @@ } } - public synchronized void incRef() { - super.incRef(); - Iterator it = norms.values().iterator(); - while (it.hasNext()) { - ((Norm) it.next()).incRef(); - } - if (deletedDocsRef != null) { - deletedDocsRef.incRef(); - } - } - - private synchronized void incRefReaderNotNorms() { - super.incRef(); - } - - public synchronized void decRef() throws IOException { - super.decRef(); - Iterator it = norms.values().iterator(); - while (it.hasNext()) { - ((Norm) it.next()).decRef(); - } - - if (deletedDocsRef != null) { - deletedDocsRef.decRef(); - } - } - - private synchronized void decRefReaderNotNorms() throws IOException { - super.decRef(); - } - Map norms = new HashMap(); /** The class which implements SegmentReader. */ @@ -690,6 +663,8 @@ try { clone.readOnly = openReadOnly; clone.directory = directory; + coreRef.incRef(); + clone.coreRef = coreRef; clone.si = si; clone.segment = segment; clone.readBufferSize = readBufferSize; @@ -706,19 +681,19 @@ clone.fieldsReaderOrig = (FieldsReader) fieldsReaderOrig.clone(); } - if (deletedDocsRef != null) { - deletedDocsRef.incRef(); - } if (doClone) { - clone.deletedDocs = deletedDocs; - clone.deletedDocsRef = deletedDocsRef; + if (deletedDocs != null) { + deletedDocsRef.incRef(); + clone.deletedDocs = deletedDocs; + clone.deletedDocsRef = deletedDocsRef; + } } else { if (!deletionsUpToDate) { // load deleted docs - clone.deletedDocs = null; - clone.deletedDocsRef = null; + assert clone.deletedDocs == null; clone.loadDeletedDocs(); - } else { + } else if (deletedDocs != null) { + deletedDocsRef.incRef(); clone.deletedDocs = deletedDocs; clone.deletedDocsRef = deletedDocsRef; } @@ -744,16 +719,6 @@ success = true; } finally { - if (this.referencedSegmentReader != null) { - // This reader shares resources with another SegmentReader, - // so we increment the other reader's refCount. - clone.referencedSegmentReader = this.referencedSegmentReader; - } else { - // We are the original SegmentReader - clone.referencedSegmentReader = this; - } - clone.referencedSegmentReader.incRefReaderNotNorms(); - if (!success) { // An exception occured during reopen, we have to decRef the norms // that we incRef'ed already and close singleNormsStream and FieldsReader @@ -801,17 +766,21 @@ } protected void doClose() throws IOException { - boolean hasReferencedReader = (referencedSegmentReader != null); termVectorsLocal.close(); fieldsReaderLocal.close(); - if (hasReferencedReader) { - referencedSegmentReader.decRefReaderNotNorms(); - referencedSegmentReader = null; + if (deletedDocs != null) { + deletedDocsRef.decRef(); } - if (!hasReferencedReader) { + Iterator it = norms.values().iterator(); + while (it.hasNext()) { + ((Norm) it.next()).decRef(); + } + + if (coreRef.decRef() == 0) { + // close everything, nothing is shared anymore with other readers if (tis != null) { tis.close(); @@ -869,12 +838,10 @@ // deletedDocs BitVector so decRef the current deletedDocsRef, // clone the BitVector, create a new deletedDocsRef if (deletedDocsRef.refCount() > 1) { - synchronized (deletedDocsRef) { - Ref oldRef = deletedDocsRef; - deletedDocs = cloneDeletedDocs(deletedDocs); - deletedDocsRef = new Ref(); - oldRef.decRef(); - } + Ref oldRef = deletedDocsRef; + deletedDocs = cloneDeletedDocs(deletedDocs); + deletedDocsRef = new Ref(); + oldRef.decRef(); } deletedDocsDirty = true; undeleteAll = false; Index: src/java/org/apache/lucene/util/BitVector.java =================================================================== --- src/java/org/apache/lucene/util/BitVector.java (revision 748341) +++ src/java/org/apache/lucene/util/BitVector.java (working copy) @@ -47,16 +47,15 @@ bits = new byte[(size >> 3) + 1]; } - BitVector(byte[] bits, int size, int count) { + BitVector(byte[] bits, int size) { this.bits = bits; this.size = size; - this.count = count; } public Object clone() { byte[] copyBits = new byte[bits.length]; System.arraycopy(bits, 0, copyBits, 0, bits.length); - return new BitVector(copyBits, size, count); + return new BitVector(copyBits, size); } /** Sets the value of bit to one. */