diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index b29417e5d62..ca4f7af5509 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -221,6 +221,9 @@ Bug Fixes * SOLR-9117: The first SolrCore is leaked after reload. (Jessica Cheng via shalin) +* SOLR-9116: Race condition causing occasional SolrIndexSearcher leak when SolrCore is reloaded. + (Jessica Cheng Mallet via shalin) + Optimizations ---------------------- * SOLR-8722: Don't force a full ZkStateReader refresh on every Overseer operation. diff --git a/solr/core/src/java/org/apache/solr/core/SolrCore.java b/solr/core/src/java/org/apache/solr/core/SolrCore.java index d5cde1669a9..b793ee7b512 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/core/src/java/org/apache/solr/core/SolrCore.java @@ -1410,6 +1410,18 @@ public final class SolrCore implements SolrInfoMBean, Closeable { private RefCounted realtimeSearcher; private Callable newReaderCreator; + // For testing + boolean areAllSearcherReferencesEmpty() { + boolean isEmpty; + synchronized (searcherLock) { + isEmpty = _searchers.isEmpty(); + isEmpty = isEmpty && _realtimeSearchers.isEmpty(); + isEmpty = isEmpty && (_searcher == null); + isEmpty = isEmpty && (realtimeSearcher == null); + } + return isEmpty; + } + /** * Return a registered {@link RefCounted}<{@link SolrIndexSearcher}> with * the reference count incremented. It must be decremented when no longer needed. @@ -1609,6 +1621,14 @@ public final class SolrCore implements SolrInfoMBean, Closeable { newSearcher.incref(); synchronized (searcherLock) { + // Check if the core is closed again inside the lock in case this method is racing with a close. If the core is + // closed, clean up the new searcher and bail. + if (isClosed()) { + newSearcher.decref(); // once for caller since we're not returning it + newSearcher.decref(); // once for ourselves since it won't be "replaced" + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "openNewSearcher called on closed core"); + } + if (realtimeSearcher != null) { realtimeSearcher.decref(); } diff --git a/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java b/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java index 2dfe025af6f..049d5e72145 100644 --- a/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java +++ b/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java @@ -17,6 +17,7 @@ package org.apache.solr.core; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.handler.ReplicationHandler; import org.apache.solr.handler.RequestHandlerBase; @@ -25,7 +26,9 @@ import org.apache.solr.handler.component.SpellCheckComponent; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.request.SolrRequestHandler; import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.search.SolrIndexSearcher; import org.apache.solr.util.DefaultSolrThreadFactory; +import org.apache.solr.util.RefCounted; import org.apache.solr.util.plugin.SolrCoreAware; import org.junit.Test; @@ -35,7 +38,6 @@ import java.util.List; import java.util.Map; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -259,6 +261,63 @@ public class SolrCoreTest extends SolrTestCaseJ4 { assertEquals("wrong config for queryResultWindowSize", 10, solrConfig.queryResultWindowSize); } + /** + * Test that's meant to be run with many iterations to expose a leak of SolrIndexSearcher when a core is closed + * due to a reload. Without the fix, this test fails with most iters=1000 runs. + */ + @Test + public void testReloadLeak() throws Exception { + final ExecutorService executor = + ExecutorUtil.newMDCAwareFixedThreadPool(1, new DefaultSolrThreadFactory("testReloadLeak")); + + // Continuously open new searcher while core is not closed, and reload core to try to reproduce searcher leak. + // While in practice we never continuously open new searchers, this is trying to make up for the fact that opening + // a searcher in this empty core is very fast by opening new searchers continuously to increase the likelihood + // for race. + SolrCore core = h.getCore(); + assertTrue("Refcount != 1", core.getOpenCount() == 1); + executor.execute(new NewSearcherRunnable(core)); + + // Since we called getCore() vs getCoreInc() and don't own a refCount, the container should decRef the core + // and close it when we call reload. + h.reload(); + + executor.shutdown(); + executor.awaitTermination(1, TimeUnit.MINUTES); + + // Check that all cores are closed and no searcher references are leaked. + assertTrue("SolrCore " + core + " is not closed", core.isClosed()); + assertTrue(core.areAllSearcherReferencesEmpty()); + } + + private static class NewSearcherRunnable implements Runnable { + private final SolrCore core; + + NewSearcherRunnable(SolrCore core) { + this.core = core; + } + + @Override + public void run() { + while (!core.isClosed()) { + try { + RefCounted newSearcher = null; + try { + newSearcher = core.openNewSearcher(true, true); + } finally { + if (newSearcher != null) { + newSearcher.decref(); + } + } + } catch (SolrException e) { + if (!core.isClosed()) { + throw e; + } + } + } + } + } + }