Index: lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java =================================================================== --- lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java (revision 1243506) +++ lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java (working copy) @@ -31,9 +31,9 @@ import org.apache.lucene.index.ConcurrentMergeScheduler; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.Term; import org.apache.lucene.index.ThreadedIndexingAndSearchingTestCase; -import org.apache.lucene.search.SearcherFactory; import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; import org.apache.lucene.util.NamedThreadFactory; @@ -55,7 +55,7 @@ if (!isNRT) { writer.commit(); } - assertTrue(mgr.maybeReopen() || mgr.isSearcherCurrent()); + assertTrue(mgr.maybeRefresh() || mgr.isSearcherCurrent()); return mgr.acquire(); } @@ -101,7 +101,7 @@ Thread.sleep(_TestUtil.nextInt(random, 1, 100)); writer.commit(); Thread.sleep(_TestUtil.nextInt(random, 1, 5)); - if (mgr.maybeReopen()) { + if (mgr.maybeRefresh()) { lifetimeMGR.prune(pruner); } } @@ -130,7 +130,7 @@ // synchronous to your search threads, but still we // test as apps will presumably do this for // simplicity: - if (mgr.maybeReopen()) { + if (mgr.maybeRefresh()) { lifetimeMGR.prune(pruner); } } @@ -196,6 +196,7 @@ final AtomicBoolean triedReopen = new AtomicBoolean(false); final ExecutorService es = random.nextBoolean() ? null : Executors.newCachedThreadPool(new NamedThreadFactory("testIntermediateClose")); final SearcherFactory factory = new SearcherFactory() { + @Override public IndexSearcher newSearcher(IndexReader r) throws IOException { try { if (triedReopen.get()) { @@ -232,7 +233,7 @@ if (VERBOSE) { System.out.println("NOW call maybeReopen"); } - searcherManager.maybeReopen(); + searcherManager.maybeRefresh(); success.set(true); } catch (AlreadyClosedException e) { // expected @@ -275,4 +276,41 @@ es.awaitTermination(1, TimeUnit.SECONDS); } } + + public void testCloseTwice() throws Exception { + // test that we can close SM twice (per Closeable's contract). + Directory dir = newDirectory(); + new IndexWriter(dir, new IndexWriterConfig(TEST_VERSION_CURRENT, null)).close(); + SearcherManager sm = new SearcherManager(dir, null); + sm.close(); + sm.close(); + dir.close(); + } + + public void testEnsureOpen() throws Exception { + Directory dir = newDirectory(); + new IndexWriter(dir, new IndexWriterConfig(TEST_VERSION_CURRENT, null)).close(); + SearcherManager sm = new SearcherManager(dir, null); + IndexSearcher s = sm.acquire(); + sm.close(); + + // this should succeed; + sm.release(s); + + try { + // this should fail + sm.acquire(); + } catch (AlreadyClosedException e) { + // ok + } + + try { + // this should fail + sm.maybeRefresh(); + } catch (AlreadyClosedException e) { + // ok + } + dir.close(); + } + } Index: lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java (revision 0) +++ lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java (working copy) @@ -0,0 +1,164 @@ +package org.apache.lucene.search; + +/** + * 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 java.io.Closeable; +import java.io.IOException; +import java.util.concurrent.Semaphore; + +import org.apache.lucene.store.AlreadyClosedException; + +/** + * Utility class to safely share instances of a certain type across multiple + * threads, while periodically refreshing them. This class ensures each + * reference is closed only once all threads have finished using it. It is + * recommended to consult the documentation of {@link ReferenceManager} + * implementations for their {@link #maybeRefresh()} semantics. + * + * @param + * the concrete type that will be {@link #acquire() acquired} and + * {@link #release(Object) released}. + * + * @lucene.experimental + */ +public abstract class ReferenceManager implements Closeable { + + private static final String REFERENCE_MANAGER_IS_CLOSED_MSG = "this ReferenceManager is closed"; + + protected volatile G current; + + private final Semaphore reopenLock = new Semaphore(1); + + private void ensureOpen() { + if (current == null) { + throw new AlreadyClosedException(REFERENCE_MANAGER_IS_CLOSED_MSG); + } + } + + private synchronized void swapReference(G newReference) throws IOException { + ensureOpen(); + final G oldReference = current; + current = newReference; + release(oldReference); + } + + /** Decrement reference counting on the given reference. */ + protected abstract void decRef(G reference) throws IOException; + + /** + * Refresh the given reference if needed. Returns {@code null} if no refresh + * was needed, otherwise a new refreshed reference. + */ + protected abstract G refreshIfNeeded(G referenceToRefresh) throws IOException; + + /** + * Try to increment reference counting on the given reference. Return true if + * the operation was successful. + */ + protected abstract boolean tryIncRef(G reference); + + /** + * Obtain the current reference. You must match every call to acquire with one + * call to {@link #release}; it's best to do so in a finally clause, and set + * the reference to {@code null} to prevent accidental usage after it has been + * released. + */ + public final G acquire() { + G ref; + do { + if ((ref = current) == null) { + throw new AlreadyClosedException(REFERENCE_MANAGER_IS_CLOSED_MSG); + } + } while (!tryIncRef(ref)); + return ref; + } + + /** + * Close this ReferenceManager to future {@link #acquire() acquiring}. Any + * references that were previously {@link #acquire() acquired} won't be + * affected, and they should still be {@link #release released} when they are + * not needed anymore. + */ + public final synchronized void close() throws IOException { + if (current != null) { + // make sure we can call this more than once + // closeable javadoc says: + // if this is already closed then invoking this method has no effect. + swapReference(null); + } + } + + /** + * You must call this, periodically, if you want that {@link #acquire()} will + * return refreshed instances. + * + *

+ * Threads: it's fine for more than one thread to call this at once. + * Only the first thread will attempt the refresh; subsequent threads will see + * that another thread is already handling refresh and will return + * immediately. Note that this means if another thread is already refreshing + * then subsequent threads will return right away without waiting for the + * refresh to complete. + * + *

+ * This method returns true if the reference was in fact refreshed, or if the + * current reference has no pending changes. + */ + public final boolean maybeRefresh() throws IOException { + ensureOpen(); + // Ensure only 1 thread does reopen at once; other threads just return immediately: + if (reopenLock.tryAcquire()) { + try { + final G reference = acquire(); + try { + G newReference = refreshIfNeeded(reference); + if (newReference != null) { + assert newReference != reference : "refreshIfNeeded should return null if refresh wasn't needed"; + boolean success = false; + try { + swapReference(newReference); + success = true; + } finally { + if (!success) { + release(newReference); + } + } + } + } finally { + release(reference); + } + return true; + } finally { + reopenLock.release(); + } + } else { + return false; + } + } + + /** + * Release the refernce previously obtained via {@link #acquire()}. + *

+ * NOTE: it's safe to call this after {@link #close()}. + */ + public final void release(G reference) throws IOException { + assert reference != null; + decRef(reference); + } + +} Property changes on: lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java ___________________________________________________________________ Added: svn:executable ## -0,0 +1 ## +* Added: svn:eol-style ## -0,0 +1 ## +native Index: lucene/core/src/java/org/apache/lucene/search/SearcherManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/SearcherManager.java (revision 1243506) +++ lucene/core/src/java/org/apache/lucene/search/SearcherManager.java (working copy) @@ -17,16 +17,10 @@ * limitations under the License. */ -import java.io.Closeable; import java.io.IOException; -import java.util.concurrent.Semaphore; -import org.apache.lucene.index.CorruptIndexException; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.IndexWriter; -import org.apache.lucene.search.NRTManager; // javadocs -import org.apache.lucene.search.IndexSearcher; // javadocs -import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; /** @@ -50,27 +44,19 @@ * * *

- * In addition you should periodically call {@link #maybeReopen}. While it's + * In addition you should periodically call {@link #maybeRefresh}. While it's * possible to call this just before running each query, this is discouraged * since it penalizes the unlucky queries that do the reopen. It's better to use * a separate background thread, that periodically calls maybeReopen. Finally, * be sure to call {@link #close} once you are done. * - *

- * NOTE: if you have an {@link IndexWriter}, it's better to use - * {@link NRTManager} since that class pulls near-real-time readers from the - * IndexWriter. - * * @see SearcherFactory * * @lucene.experimental */ +public final class SearcherManager extends ReferenceManager { -public final class SearcherManager implements Closeable { - - private volatile IndexSearcher currentSearcher; private final SearcherFactory searcherFactory; - private final Semaphore reopenLock = new Semaphore(1); /** * Creates and returns a new SearcherManager from the given {@link IndexWriter}. @@ -93,8 +79,28 @@ searcherFactory = new SearcherFactory(); } this.searcherFactory = searcherFactory; - currentSearcher = searcherFactory.newSearcher(IndexReader.open(writer, applyAllDeletes)); + current = searcherFactory.newSearcher(IndexReader.open(writer, applyAllDeletes)); } + + @Override + protected void decRef(IndexSearcher reference) throws IOException { + reference.getIndexReader().decRef(); + } + + @Override + protected IndexSearcher refreshIfNeeded(IndexSearcher referenceToRefresh) throws IOException { + final IndexReader newReader = IndexReader.openIfChanged(referenceToRefresh.getIndexReader()); + if (newReader == null) { + return null; + } else { + return searcherFactory.newSearcher(newReader); + } + } + + @Override + protected boolean tryIncRef(IndexSearcher reference) { + return reference.getIndexReader().tryIncRef(); + } /** * Creates and returns a new SearcherManager from the given {@link Directory}. @@ -110,72 +116,15 @@ searcherFactory = new SearcherFactory(); } this.searcherFactory = searcherFactory; - currentSearcher = searcherFactory.newSearcher(IndexReader.open(dir)); + current = searcherFactory.newSearcher(IndexReader.open(dir)); } /** - * You must call this, periodically, to perform a reopen. This calls - * {@link IndexReader#openIfChanged(IndexReader)} with the underlying reader, and if that returns a - * new reader, it's warmed (if you provided a {@link SearcherFactory} and then - * swapped into production. - * - *

- * Threads: it's fine for more than one thread to call this at once. - * Only the first thread will attempt the reopen; subsequent threads will see - * that another thread is already handling reopen and will return immediately. - * Note that this means if another thread is already reopening then subsequent - * threads will return right away without waiting for the reader reopen to - * complete. - *

- * - *

- * This method returns true if a new reader was in fact opened or - * if the current searcher has no pending changes. - *

- */ - public boolean maybeReopen() throws IOException { - ensureOpen(); - // Ensure only 1 thread does reopen at once; other - // threads just return immediately: - if (reopenLock.tryAcquire()) { - try { - // IR.openIfChanged preserves NRT and applyDeletes - // in the newly returned reader: - final IndexReader newReader; - final IndexSearcher searcherToReopen = acquire(); - try { - newReader = IndexReader.openIfChanged(searcherToReopen.getIndexReader()); - } finally { - release(searcherToReopen); - } - if (newReader != null) { - final IndexSearcher newSearcher = searcherFactory.newSearcher(newReader); - boolean success = false; - try { - swapSearcher(newSearcher); - success = true; - } finally { - if (!success) { - release(newSearcher); - } - } - } - return true; - } finally { - reopenLock.release(); - } - } else { - return false; - } - } - - /** * Returns true if no changes have occured since this searcher * ie. reader was opened, otherwise false. * @see IndexReader#isCurrent() */ - public boolean isSearcherCurrent() throws CorruptIndexException, - IOException { + public boolean isSearcherCurrent() throws IOException { final IndexSearcher searcher = acquire(); try { return searcher.getIndexReader().isCurrent(); @@ -183,56 +132,5 @@ release(searcher); } } - - /** - * Release the searcher previously obtained with {@link #acquire}. - * - *

- * NOTE: it's safe to call this after {@link #close}. - */ - public void release(IndexSearcher searcher) throws IOException { - assert searcher != null; - searcher.getIndexReader().decRef(); - } - - /** - * Close this SearcherManager to future searching. Any searches still in - * process in other threads won't be affected, and they should still call - * {@link #release} after they are done. - */ - public synchronized void close() throws IOException { - if (currentSearcher != null) { - // make sure we can call this more than once - // closeable javadoc says: - // if this is already closed then invoking this method has no effect. - swapSearcher(null); - } - } - - /** - * Obtain the current IndexSearcher. You must match every call to acquire with - * one call to {@link #release}; it's best to do so in a finally clause. - */ - public IndexSearcher acquire() { - IndexSearcher searcher; - do { - if ((searcher = currentSearcher) == null) { - throw new AlreadyClosedException("this SearcherManager is closed"); - } - } while (!searcher.getIndexReader().tryIncRef()); - return searcher; - } - - private void ensureOpen() { - if (currentSearcher == null) { - throw new AlreadyClosedException("this SearcherManager is closed"); - } - } - - private synchronized void swapSearcher(IndexSearcher newSearcher) throws IOException { - ensureOpen(); - final IndexSearcher oldSearcher = currentSearcher; - currentSearcher = newSearcher; - release(oldSearcher); - } + } Index: lucene/core/src/java/org/apache/lucene/search/NRTManager.java =================================================================== --- lucene/core/src/java/org/apache/lucene/search/NRTManager.java (revision 1243506) +++ lucene/core/src/java/org/apache/lucene/search/NRTManager.java (working copy) @@ -305,7 +305,7 @@ return false; } if (!(setSearchGen = reference.manager.isSearcherCurrent())) { - setSearchGen = reference.manager.maybeReopen(); + setSearchGen = reference.manager.maybeRefresh(); } if (setSearchGen) { reference.generation = newSearcherGen;// update searcher gen Index: lucene/CHANGES.txt =================================================================== --- lucene/CHANGES.txt (revision 1243506) +++ lucene/CHANGES.txt (working copy) @@ -69,7 +69,12 @@ IndexCommit.getTimestamp and .getVersion and IndexReader.lastModified and getCurrentVersion (Andrzej Bialecki, Robert Muir, Mike McCandless) - + +* LUCENE-3761: Generalize SearcherManager into an abstract ReferenceManager. + SearcherManager remains a concrete class, but due to the refactoring, the + method maybeReopen was changed to maybeRefresh(). + (Shai Erera, Mike McCandless, Simon Willnauer) + Security fixes * LUCENE-3588: Try harder to prevent SIGSEGV on cloned MMapIndexInputs: