iprithv commented on code in PR #16152:
URL: https://github.com/apache/lucene/pull/16152#discussion_r3448968527
##########
lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestSearcherTaxonomyManager.java:
##########
@@ -420,18 +420,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);
Review Comment:
This doesn't test anything, it's asserting a constant against itself.
leftover from the old test, should just be deleted.
##########
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) {
Review Comment:
I'm sorry but this doesn't fix the race described in #15831. race is about
Thread B's maybeRefresh() getting skipped entirely because tryLock() returns
false, that happens before we ever enter doMaybeRefresh(). Looping inside
doMaybeRefresh only helps the thread that already holds the lock, but that
thread has no way to see the generation that Thread B hasn't flushed yet.
Also this loop is risky: if a custom refreshIfNeeded implementation ever has
a bug where it doesn't return null, this becomes an infinite loop under the
refresh lock, blocking everything. The current single shot design is safer.
On top of that, notifyRefreshListenersBefore() is called once before the
loop but the loop can swap references multiple times. Listeners expect a 1:1
before/after pairing per refresh, this breaks that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
