You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by sh...@apache.org on 2011/10/05 14:01:17 UTC
svn commit: r1179183 - in /lucene/dev/branches/branch_3x/lucene/contrib:
CHANGES.txt
facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java
facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java
Author: shaie
Date: Wed Oct 5 12:01:17 2011
New Revision: 1179183
URL: http://svn.apache.org/viewvc?rev=1179183&view=rev
Log:
LUCENE-3485: LuceneTaxonomyReader .decRef() may close the inner IR, renderring the LTR in a limbo
Added:
lucene/dev/branches/branch_3x/lucene/contrib/facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java (with props)
Modified:
lucene/dev/branches/branch_3x/lucene/contrib/CHANGES.txt
lucene/dev/branches/branch_3x/lucene/contrib/facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java
Modified: lucene/dev/branches/branch_3x/lucene/contrib/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/contrib/CHANGES.txt?rev=1179183&r1=1179182&r2=1179183&view=diff
==============================================================================
--- lucene/dev/branches/branch_3x/lucene/contrib/CHANGES.txt (original)
+++ lucene/dev/branches/branch_3x/lucene/contrib/CHANGES.txt Wed Oct 5 12:01:17 2011
@@ -54,6 +54,10 @@ Bug Fixes
* LUCENE-3484: Fix NPE in TaxonomyWriter: parents array creation was not thread safe.
(Doron Cohen)
+
+ * LUCENE-3485: Fix a bug in LuceneTaxonomyReader, where calling decRef() might
+ close the inner IndexReader, leaving the taxonomy reader in limbo.
+ (Gilad Barkai via Shai Erera)
API Changes
Modified: lucene/dev/branches/branch_3x/lucene/contrib/facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/contrib/facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java?rev=1179183&r1=1179182&r2=1179183&view=diff
==============================================================================
--- lucene/dev/branches/branch_3x/lucene/contrib/facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java (original)
+++ lucene/dev/branches/branch_3x/lucene/contrib/facet/src/java/org/apache/lucene/facet/taxonomy/lucene/LuceneTaxonomyReader.java Wed Oct 5 12:01:17 2011
@@ -1,6 +1,5 @@
package org.apache.lucene.facet.taxonomy.lucene;
-import java.io.File;
import java.io.IOException;
import java.util.Iterator;
import java.util.Map;
@@ -10,15 +9,14 @@ import java.util.concurrent.locks.Reentr
import java.util.logging.Level;
import java.util.logging.Logger;
+import org.apache.lucene.facet.taxonomy.CategoryPath;
+import org.apache.lucene.facet.taxonomy.TaxonomyReader;
import org.apache.lucene.index.CorruptIndexException;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.Term;
import org.apache.lucene.index.TermDocs;
+import org.apache.lucene.store.AlreadyClosedException;
import org.apache.lucene.store.Directory;
-import org.apache.lucene.store.FSDirectory;
-
-import org.apache.lucene.facet.taxonomy.CategoryPath;
-import org.apache.lucene.facet.taxonomy.TaxonomyReader;
import org.apache.lucene.util.collections.LRUHashMap;
/**
@@ -98,6 +96,8 @@ public class LuceneTaxonomyReader implem
private char delimiter = Consts.DEFAULT_DELIMITER;
+ private volatile boolean closed = false;
+
/**
* Open for reading a taxonomy stored in a given {@link Directory}.
* @param directory
@@ -125,40 +125,15 @@ public class LuceneTaxonomyReader implem
return IndexReader.open(directory);
}
- // convenience constructors... deprecated because they cause confusion
- // because they use parent directory instead of the actual directory.
- private Directory ourDirectory = null; // remember directory to close later, but only if we opened it here
/**
- * Open for reading a taxonomy stored in a subdirectory of a given
- * directory on the file system.
- * @param parentDir The parent directory of the taxonomy's directory
- * (usually this would be the directory holding the index).
- * @param name The name of the taxonomy, and the subdirectory holding it.
- * @throws CorruptIndexException if the Taxonomy is corrupted.
- * @throws IOException if another error occurred.
- */
- @Deprecated
- public LuceneTaxonomyReader(File parentDir, String name)
- throws CorruptIndexException, IOException {
- this(FSDirectory.open(new File(parentDir, name)));
- ourDirectory = indexReader.directory(); // remember to close the directory we opened
- }
-
- /**
- * Open for reading a taxonomy stored in a subdirectory of a given
- * directory on the file system.
- * @param parentDir The parent directory of the taxonomy's directory.
- * @param name The name of the taxonomy, and the subdirectory holding it.
- * @throws CorruptIndexException if the Taxonomy is corrupted.
- * @throws IOException if another error occurred.
- */
- @Deprecated
- public LuceneTaxonomyReader(String parentDir, String name)
- throws CorruptIndexException, IOException {
- this(FSDirectory.open(new File(parentDir, name)));
- ourDirectory = indexReader.directory(); // rememebr to close the directory we opened
+ * @throws AlreadyClosedException if this IndexReader is closed
+ */
+ protected final void ensureOpen() throws AlreadyClosedException {
+ if (indexReader.getRefCount() <= 0) {
+ throw new AlreadyClosedException("this TaxonomyReader is closed");
+ }
}
-
+
/**
* setCacheSize controls the maximum allowed size of each of the caches
* used by {@link #getPath(int)} and {@link #getOrdinal(CategoryPath)}.
@@ -169,6 +144,7 @@ public class LuceneTaxonomyReader implem
* @param size the new maximum cache size, in number of entries.
*/
public void setCacheSize(int size) {
+ ensureOpen();
synchronized(getCategoryCache) {
getCategoryCache.setMaxSize(size);
}
@@ -188,10 +164,12 @@ public class LuceneTaxonomyReader implem
* LuceneTaxonomyReader objects you create.
*/
public void setDelimiter(char delimiter) {
+ ensureOpen();
this.delimiter = delimiter;
}
public int getOrdinal(CategoryPath categoryPath) throws IOException {
+ ensureOpen();
if (categoryPath.length()==0) {
return ROOT_ORDINAL;
}
@@ -233,6 +211,7 @@ public class LuceneTaxonomyReader implem
}
public CategoryPath getPath(int ordinal) throws CorruptIndexException, IOException {
+ ensureOpen();
// TODO (Facet): Currently, the LRU cache we use (getCategoryCache) holds
// strings with delimiters, not CategoryPath objects, so even if
// we have a cache hit, we need to process the string and build a new
@@ -249,6 +228,7 @@ public class LuceneTaxonomyReader implem
}
public boolean getPath(int ordinal, CategoryPath result) throws CorruptIndexException, IOException {
+ ensureOpen();
String label = getLabel(ordinal);
if (label==null) {
return false;
@@ -259,6 +239,7 @@ public class LuceneTaxonomyReader implem
}
private String getLabel(int catID) throws CorruptIndexException, IOException {
+ ensureOpen();
// First try to find the answer in the LRU cache. It is very
// unfortunate that we need to allocate an Integer object here -
// it would have been better if we used a hash table specifically
@@ -307,6 +288,7 @@ public class LuceneTaxonomyReader implem
}
public int getParent(int ordinal) {
+ ensureOpen();
// Note how we don't need to hold the read lock to do the following,
// because the array reference is volatile, ensuring the correct
// visibility and ordering: if we get the new reference, the new
@@ -337,6 +319,7 @@ public class LuceneTaxonomyReader implem
*/
public int[] getParentArray() {
+ ensureOpen();
// Note how we don't need to hold the read lock to do the following,
// because the array reference is volatile, ensuring the correct
// visibility and ordering: if we get the new reference, the new
@@ -348,6 +331,7 @@ public class LuceneTaxonomyReader implem
// method in this class) to ensure that it never gets called concurrently
// with itself.
public synchronized void refresh() throws IOException {
+ ensureOpen();
/*
* Since refresh() can be a lengthy operation, it is very important that we
* avoid locking out all readers for its duration. This is why we don't hold
@@ -409,13 +393,25 @@ public class LuceneTaxonomyReader implem
}
public void close() throws IOException {
- indexReader.close();
- if (ourDirectory!=null) {
- ourDirectory.close();
+ if (!closed) {
+ decRef();
+ closed = true;
}
}
+
+ /** Do the actual closing, free up resources */
+ private void doClose() throws IOException {
+ indexReader.close();
+ closed = true;
+
+ parentArray = null;
+ childrenArrays = null;
+ getCategoryCache.clear();
+ getOrdinalCache.clear();
+ }
public int getSize() {
+ ensureOpen();
indexReaderLock.readLock().lock();
try {
return indexReader.numDocs();
@@ -425,6 +421,7 @@ public class LuceneTaxonomyReader implem
}
public Map<String, String> getCommitUserData() {
+ ensureOpen();
return indexReader.getCommitUserData();
}
@@ -432,6 +429,7 @@ public class LuceneTaxonomyReader implem
Object childrenArraysRebuild = new Object();
public ChildrenArrays getChildrenArrays() {
+ ensureOpen();
// Check if the taxonomy grew since we built the array, and if it
// did, create new (and larger) arrays and fill them as required.
// We do all this under a lock, two prevent to concurrent calls to
@@ -485,6 +483,7 @@ public class LuceneTaxonomyReader implem
}
public String toString(int max) {
+ ensureOpen();
StringBuilder sb = new StringBuilder();
int upperl = Math.min(max, this.indexReader.maxDoc());
for (int i = 0; i < upperl; i++) {
@@ -530,6 +529,7 @@ public class LuceneTaxonomyReader implem
* @return lucene indexReader
*/
IndexReader getInternalIndexReader() {
+ ensureOpen();
return this.indexReader;
}
@@ -540,13 +540,20 @@ public class LuceneTaxonomyReader implem
* @throws IOException
*/
public void decRef() throws IOException {
- this.indexReader.decRef();
+ ensureOpen();
+ if (indexReader.getRefCount() == 1) {
+ // Do not decRef the indexReader - doClose does it by calling reader.close()
+ doClose();
+ } else {
+ indexReader.decRef();
+ }
}
/**
* Expert: returns the current refCount for this taxonomy reader
*/
public int getRefCount() {
+ ensureOpen();
return this.indexReader.getRefCount();
}
@@ -558,6 +565,7 @@ public class LuceneTaxonomyReader implem
* otherwise the reader may never be closed.
*/
public void incRef() {
+ ensureOpen();
this.indexReader.incRef();
}
}
Added: lucene/dev/branches/branch_3x/lucene/contrib/facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/contrib/facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java?rev=1179183&view=auto
==============================================================================
--- lucene/dev/branches/branch_3x/lucene/contrib/facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java (added)
+++ lucene/dev/branches/branch_3x/lucene/contrib/facet/src/test/org/apache/lucene/facet/taxonomy/lucene/TestLuceneTaxonomyReader.java Wed Oct 5 12:01:17 2011
@@ -0,0 +1,78 @@
+package org.apache.lucene.facet.taxonomy.lucene;
+
+import org.apache.lucene.facet.taxonomy.CategoryPath;
+import org.apache.lucene.store.AlreadyClosedException;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.LuceneTestCase;
+import org.junit.Test;
+
+/**
+ * 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.
+ */
+
+public class TestLuceneTaxonomyReader extends LuceneTestCase {
+
+ @Test
+ public void testCloseAfterIncRef() throws Exception {
+ Directory dir = newDirectory();
+ LuceneTaxonomyWriter ltw = new LuceneTaxonomyWriter(dir);
+ ltw.addCategory(new CategoryPath("a"));
+ ltw.close();
+
+ LuceneTaxonomyReader ltr = new LuceneTaxonomyReader(dir);
+ ltr.incRef();
+ ltr.close();
+
+ // should not fail as we incRef() before close
+ ltr.getSize();
+ ltr.decRef();
+
+ dir.close();
+ }
+
+ @Test
+ public void testCloseTwice() throws Exception {
+ Directory dir = newDirectory();
+ LuceneTaxonomyWriter ltw = new LuceneTaxonomyWriter(dir);
+ ltw.addCategory(new CategoryPath("a"));
+ ltw.close();
+
+ LuceneTaxonomyReader ltr = new LuceneTaxonomyReader(dir);
+ ltr.close();
+ ltr.close(); // no exception should be thrown
+
+ dir.close();
+ }
+
+ @Test
+ public void testAlreadyClosed() throws Exception {
+ Directory dir = newDirectory();
+ LuceneTaxonomyWriter ltw = new LuceneTaxonomyWriter(dir);
+ ltw.addCategory(new CategoryPath("a"));
+ ltw.close();
+
+ LuceneTaxonomyReader ltr = new LuceneTaxonomyReader(dir);
+ ltr.close();
+ try {
+ ltr.getSize();
+ fail("An AlreadyClosedException should have been thrown here");
+ } catch (AlreadyClosedException ace) {
+ // good!
+ }
+ dir.close();
+ }
+
+}