diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 4b1ba157991..dfb2acb872c 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -846,6 +846,9 @@ New features * LUCENE-4004: Add DisjunctionMaxQuery support to the xml query parser. (Benson Margulies via Robert Muir) +* LUCENE-4025: Add maybeRefreshBlocking to ReferenceManager, to let a caller + block until the refresh logic has been executed. (Shai Erera, Mike McCandless) + Optimizations * LUCENE-2588: Don't store unnecessary suffixes when writing the terms diff --git a/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java b/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java index 0febc78be73..23766774492 100755 --- a/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java @@ -19,7 +19,8 @@ package org.apache.lucene.search; import java.io.Closeable; import java.io.IOException; -import java.util.concurrent.Semaphore; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.apache.lucene.store.AlreadyClosedException; @@ -42,7 +43,7 @@ public abstract class ReferenceManager implements Closeable { protected volatile G current; - private final Semaphore reopenLock = new Semaphore(1); + private final Lock refreshLock = new ReentrantLock(); private void ensureOpen() { if (current == null) { @@ -108,9 +109,42 @@ public abstract class ReferenceManager implements Closeable { protected void afterClose() throws IOException { } + private void doMaybeRefresh() throws IOException { + // it's ok to call lock() here (blocking) because we're supposed to get here + // from either maybeRefreh() or maybeRefreshBlocking(), after the lock has + // already been obtained. Doing that protects us from an accidental bug + // where this method will be called outside the scope of refreshLock. + // Per ReentrantLock's javadoc, calling lock() by the same thread more than + // once is ok, as long as unlock() is called a matching number of times. + refreshLock.lock(); + 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); + } + afterRefresh(); + } finally { + refreshLock.unlock(); + } + } + /** - * You must call this, periodically, if you want that {@link #acquire()} will - * return refreshed instances. + * You must call this (or {@link #maybeRefreshBlocking()}), 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. @@ -121,43 +155,48 @@ public abstract class ReferenceManager implements Closeable { * refresh to complete. * *

- * If this method returns true it means the calling thread either refreshed - * or that there were no changes to refresh. If it returns false it means another + * If this method returns true it means the calling thread either refreshed or + * that there were no changes to refresh. If it returns false it means another * thread is currently refreshing. */ public final boolean maybeRefresh() throws IOException { ensureOpen(); // Ensure only 1 thread does reopen at once; other threads just return immediately: - final boolean doTryRefresh = reopenLock.tryAcquire(); + final boolean doTryRefresh = refreshLock.tryLock(); if (doTryRefresh) { 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); - } - afterRefresh(); + doMaybeRefresh(); } finally { - reopenLock.release(); + refreshLock.unlock(); } } return doTryRefresh; } + + /** + * You must call this (or {@link #maybeRefresh()}), periodically, if you want + * that {@link #acquire()} will return refreshed instances. + * + *

+ * Threads: unlike {@link #maybeRefresh()}, if another thread is + * currently refreshing, this method blocks until that thread completes. It is + * useful if you want to guarantee that the next call to {@link #acquire()} + * will return a refreshed instance. Otherwise, consider using the + * non-blocking {@link #maybeRefresh()}. + */ + public final void maybeRefreshBlocking() throws IOException, InterruptedException { + ensureOpen(); + + // Ensure only 1 thread does reopen at once + refreshLock.lock(); + try { + doMaybeRefresh(); + } finally { + refreshLock.lock(); + } + } /** Called after swapReference has installed a new * instance. */ diff --git a/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java b/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java index 746001febc9..4b4dd93bb3b 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java @@ -108,7 +108,11 @@ public class TestSearcherManager extends ThreadedIndexingAndSearchingTestCase { Thread.sleep(_TestUtil.nextInt(random(), 1, 100)); writer.commit(); Thread.sleep(_TestUtil.nextInt(random(), 1, 5)); - if (mgr.maybeRefresh()) { + boolean block = random().nextBoolean(); + if (block) { + mgr.maybeRefreshBlocking(); + lifetimeMGR.prune(pruner); + } else if (mgr.maybeRefresh()) { lifetimeMGR.prune(pruner); } }