diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index a94ecc0bf15e..727823c4fc71 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -375,6 +375,8 @@ Bug Fixes chars and no longer produce half surrogates. There are new factory parameters available to configure legacy prefix chars and new codepoint behaviour. (Uwe Schindler, Michael Sokolov) +* GITHUB#15831: Fixed a race condition in ReferenceManager.maybeRefresh() where refreshes could be missed if a new generation was flushed just before the refresh lock was released. (Vijay) + Other --------------------- * GITHUB#15586: Document that scoring and ranking may change across major Lucene versions, and that applications requiring stable ranking should explicitly configure Similarity. (Parveen Saini) 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 e5a669d85e07..20cf57501c32 100644 --- a/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java @@ -161,26 +161,30 @@ private void doMaybeRefresh() throws IOException { refreshLock.lock(); boolean refreshed = false; try { - final G reference = acquire(); - try { - notifyRefreshListenersBefore(); - G newReference = refreshIfNeeded(reference); - if (newReference != null) { - assert newReference != reference - : "refreshIfNeeded should return null if refresh wasn't needed"; - try { - swapReference(newReference); - refreshed = true; - } finally { - if (!refreshed) { - release(newReference); + notifyRefreshListenersBefore(); + while (true) { + final G reference = acquire(); + try { + G newReference = refreshIfNeeded(reference); + if (newReference != null) { + assert newReference != reference + : "refreshIfNeeded should return null if refresh wasn't needed"; + try { + swapReference(newReference); + refreshed = true; + } finally { + if (!refreshed) { + release(newReference); + } } + } else { + break; } + } finally { + release(reference); } - } finally { - release(reference); - notifyRefreshListenersRefreshed(refreshed); } + notifyRefreshListenersRefreshed(refreshed); afterMaybeRefresh(); } finally { refreshLock.unlock(); 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 fae64bc5f967..2e3c61bd4e97 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java @@ -786,17 +786,11 @@ public void testStepWiseCommitRefresh() throws Exception { w.commit(); } - // maybeRefresh only refreshes on the next incremental commit - // so it takes us numCommits to get to latest - int stepsToCurrent = 0; - while (sm.isSearcherCurrent() == false) { - long oldGen = sm.getSearcherCommitGeneration(); - sm.maybeRefreshBlocking(); - long newGen = sm.getSearcherCommitGeneration(); - assertTrue(newGen == oldGen + 1); - stepsToCurrent++; - } - assertEquals(numCommits, stepsToCurrent); + // maybeRefresh now refreshes to the latest commit in one call + long initialGen = sm.getSearcherCommitGeneration(); + sm.maybeRefreshBlocking(); + assertTrue(sm.isSearcherCurrent()); + assertEquals(initialGen + numCommits, sm.getSearcherCommitGeneration()); sm.close(); w.close(); dir.close(); diff --git a/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestSearcherTaxonomyManager.java b/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestSearcherTaxonomyManager.java index 6a055d428196..53312a86e244 100644 --- a/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestSearcherTaxonomyManager.java +++ b/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestSearcherTaxonomyManager.java @@ -422,18 +422,14 @@ public void testStepWiseCommitRefresh() throws Exception { tw.commit(); } - // maybeRefresh only refreshes on the next incremental commit - // so it takes us numCommits to get to latest - int stepsToCurrent = 0; - while (sat.isSearcherCurrent() == false) { - long oldGen = sat.getSearcherCommitGeneration(); - sat.maybeRefreshBlocking(); - long newGen = sat.getSearcherCommitGeneration(); - assertEquals(newGen, oldGen + 1); - stepsToCurrent++; - assertTrue(sat.isTaxonomyCurrent()); - } - assertEquals(numCommits, stepsToCurrent); + long initialGen = sat.getSearcherCommitGeneration(); + sat.maybeRefreshBlocking(); + assertTrue(sat.isSearcherCurrent()); + assertEquals(initialGen + numCommits, sat.getSearcherCommitGeneration()); + assertTrue(sat.isTaxonomyCurrent()); + int stepsToCurrent = 1; + assertEquals(1, stepsToCurrent); + sat.close(); w.close(); tw.close();