Fix(15831): Resolve race condition in ReferenceManager.doMaybeRefresh

lucene/CHANGES.txt

@@ -179,6 +179,8 @@ Bug Fixes
 
 * GITHUB#15939: Fix thread-safety issues with NFARunAutomaton. (Dimitris Rempapis)
 
+* 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)
+
 Changes in Runtime Behavior
 ---------------------
 * GITHUB#14187: The query cache is now disabled by default. (Adrien Grand)

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();

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();

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();