diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6044-fix-issue-with-cache-refresh-infinite-loop.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6044-fix-issue-with-cache-refresh-infinite-loop.yaml new file mode 100644 index 00000000000..a883f4d37ae --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6044-fix-issue-with-cache-refresh-infinite-loop.yaml @@ -0,0 +1,7 @@ +--- +type: fix +issue: 6044 +title: "Fixed an issue where doing a cache refresh with advanced Hibernate Search + enabled would result in an infinite loop of cache refresh -> search for + StructureDefinition -> cache refresh, etc +" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java index 1a43905b822..0e436fccf10 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java @@ -151,6 +151,15 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { return true; } + // if the registry has not been initialized + // we cannot use HibernateSearch because it + // will, internally, trigger a new search + // when it refreshes the search parameters + // (which will cause an infinite loop) + if (!mySearchParamRegistry.isInitialized()) { + return false; + } + return myStorageSettings.isAdvancedHSearchIndexing() && myAdvancedIndexQueryBuilder.canUseHibernateSearch(theResourceType, myParams, mySearchParamRegistry); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java index 9f6b6c88261..5b9634b6a80 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java @@ -89,6 +89,7 @@ public class ExtendedHSearchSearchBuilder { public boolean canUseHibernateSearch( String theResourceType, SearchParameterMap myParams, ISearchParamRegistry theSearchParamRegistry) { boolean canUseHibernate = true; + ResourceSearchParams resourceActiveSearchParams = theSearchParamRegistry.getActiveSearchParams(theResourceType); for (String paramName : myParams.keySet()) { // is this parameter supported? diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java index 002fcf8f640..a64b938e466 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java @@ -43,6 +43,9 @@ import java.time.ZoneId; @Scope("prototype") public class ResourceChangeListenerCache implements IResourceChangeListenerCache { private static final Logger ourLog = LoggerFactory.getLogger(ResourceChangeListenerCache.class); + /** + * Max number of retries to do for cache refreshing + */ private static final int MAX_RETRIES = 60; private static Instant ourNowForUnitTests; @@ -123,7 +126,7 @@ public class ResourceChangeListenerCache implements IResourceChangeListenerCache return myNextRefreshTime.isBefore(now()); } - private static Instant now() { + static Instant now() { if (ourNowForUnitTests != null) { return ourNowForUnitTests; } @@ -153,7 +156,7 @@ public class ResourceChangeListenerCache implements IResourceChangeListenerCache return myResourceChangeListenerCacheRefresher.refreshCacheAndNotifyListener(this); } }, - MAX_RETRIES); + getMaxRetries()); return refreshCacheRetrier.runWithRetry(); } @@ -223,4 +226,8 @@ public class ResourceChangeListenerCache implements IResourceChangeListenerCache .append("myInitialized", myInitialized) .toString(); } + + static int getMaxRetries() { + return MAX_RETRIES; + } } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java index 38ceb2b24b8..53c3be48fc1 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java @@ -149,6 +149,7 @@ public class ResourceChangeListenerCacheRefresherImpl return retVal; } SearchParameterMap searchParamMap = theCache.getSearchParameterMap(); + ResourceVersionMap newResourceVersionMap = myResourceVersionSvc.getVersionMap(theCache.getResourceName(), searchParamMap); diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java index 4d67c566bc1..9670b18f175 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java @@ -132,6 +132,7 @@ public class SearchParamRegistryImpl private void requiresActiveSearchParams() { if (myActiveSearchParams == null) { + // forced refreshes should not use a cache - we're forcibly refrsching it, after all myResourceChangeListenerCache.forceRefresh(); } } @@ -230,7 +231,7 @@ public class SearchParamRegistryImpl } } - myActiveSearchParams = searchParams; + setActiveSearchParams(searchParams); myJpaSearchParamCache.populateActiveSearchParams( myInterceptorBroadcaster, myPhoneticEncoder, myActiveSearchParams); @@ -432,9 +433,15 @@ public class SearchParamRegistryImpl initializeActiveSearchParams(searchParams); } + @Override + public boolean isInitialized() { + return myActiveSearchParams != null; + } + @VisibleForTesting public void resetForUnitTest() { myBuiltInSearchParams = null; + setActiveSearchParams(null); handleInit(Collections.emptyList()); } @@ -448,4 +455,9 @@ public class SearchParamRegistryImpl public int getMaxManagedParamCountForUnitTests() { return MAX_MANAGED_PARAM_COUNT; } + + @VisibleForTesting + public void setActiveSearchParams(RuntimeSearchParamCache theSearchParams) { + myActiveSearchParams = theSearchParams; + } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerRegistryImplIT.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerRegistryImplIT.java index cf027083301..8dd082530c2 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerRegistryImplIT.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerRegistryImplIT.java @@ -1,11 +1,9 @@ package ca.uhn.fhir.jpa.cache; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.interceptor.api.HookParams; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.jpa.searchparam.registry.SearchParamRegistryImpl; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.param.DateRangeParam; @@ -20,6 +18,7 @@ import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -27,10 +26,16 @@ import org.springframework.beans.factory.annotation.Autowired; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.mockStatic; public class ResourceChangeListenerRegistryImplIT extends BaseJpaR4Test { @@ -40,6 +45,9 @@ public class ResourceChangeListenerRegistryImplIT extends BaseJpaR4Test { @Autowired IResourceChangeListenerCacheRefresher myResourceChangeListenerCacheRefresher; + @Autowired + private SearchParamRegistryImpl mySearchParamRegistry; + private final static String RESOURCE_NAME = "Patient"; private TestCallback myMaleTestCallback = new TestCallback("MALE"); private TestCallback myFemaleTestCallback = new TestCallback("FEMALE"); @@ -55,6 +63,63 @@ public class ResourceChangeListenerRegistryImplIT extends BaseJpaR4Test { myResourceChangeListenerRegistry.clearCachesForUnitTest(); } + @Test + public void forceRefresh_beforeSearchParametersInitialized_willNotFallIntoAnInfiniteLoop() { + // setup + int maxRetries = 3; // a small number for short tests + AtomicInteger counter = new AtomicInteger(); + AtomicBoolean initCheck = new AtomicBoolean(); + + IResourceChangeListenerCache cache = myResourceChangeListenerRegistry.registerResourceResourceChangeListener( + "StructureDefinition", + SearchParameterMap.newSynchronous(), + new IResourceChangeListener() { + @Override + public void handleInit(Collection theResourceIds) { + initCheck.set(true); + } + + @Override + public void handleChange(IResourceChangeEvent theResourceChangeEvent) { + + } + }, + 100 + ); + + assertTrue((cache instanceof ResourceChangeListenerCache)); + + // set the HSearchIndexing + boolean useAdvancedHSearch = myStorageSettings.isAdvancedHSearchIndexing(); + myStorageSettings.setAdvancedHSearchIndexing(true); + + // so they will be forced to be refreshed + mySearchParamRegistry.setActiveSearchParams(null); + try (MockedStatic cacheConstants = mockStatic(ResourceChangeListenerCache.class)) { + cacheConstants.when(ResourceChangeListenerCache::getMaxRetries).thenAnswer((args) -> { + if (counter.getAndIncrement() > maxRetries) { + // fail after a few tries to ensure we don't fall into an infinite loop + fail("This should not fall into an infinite loop"); + } + return maxRetries; + }); + cacheConstants.when(ResourceChangeListenerCache::now) + .thenCallRealMethod(); + + // test + ResourceChangeResult result = cache.forceRefresh(); + + // verify + assertEquals(0, result.created); + assertEquals(0, result.updated); + assertEquals(0, result.deleted); + assertTrue(initCheck.get()); + } finally { + // reset for other tests + myStorageSettings.setAdvancedHSearchIndexing(useAdvancedHSearch); + } + } + @Test public void testRegisterListener() throws InterruptedException { assertEquals(0, myResourceChangeListenerRegistry.getResourceVersionCacheSizeForUnitTest()); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index c39d3994050..9d6150deb28 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java @@ -1,6 +1,5 @@ package ca.uhn.fhir.jpa.dao.r4; -import static org.junit.jupiter.api.Assertions.assertNull; import ca.uhn.fhir.batch2.api.IJobDataSink; import ca.uhn.fhir.batch2.api.RunOutcome; import ca.uhn.fhir.batch2.api.VoidModel; @@ -106,14 +105,13 @@ import java.util.stream.IntStream; import static ca.uhn.fhir.jpa.subscription.FhirR4Util.createSubscription; import static org.apache.commons.lang3.StringUtils.countMatches; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.fail; import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; - import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -155,7 +153,6 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test protected SubscriptionTestUtil mySubscriptionTestUtil; private ReindexTestHelper myReindexTestHelper; - @AfterEach public void afterResetDao() { mySubscriptionSettings.clearSupportedSubscriptionTypesForUnitTest(); diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/ISearchParamRegistry.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/ISearchParamRegistry.java index b228ac703d6..12b628fe641 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/ISearchParamRegistry.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/ISearchParamRegistry.java @@ -39,6 +39,16 @@ import java.util.TreeSet; // TODO: JA remove default methods public interface ISearchParamRegistry { + /** + * Return true if this registry is initialized and ready to handle + * searches and use its cache. + * Return false if cache has not been initialized. + */ + default boolean isInitialized() { + // default initialized to not break current implementers + return true; + } + /** * @return Returns {@literal null} if no match */