fixing issue with infinite cache refresh loop (hapifhir#6045)

* fixing step 1 * spotless * cleanup * cleanup * cleanup * review fixes * review fix --------- Co-authored-by: leif stawnyczy <[email protected]>
formelio · Jun 27, 2024 · 90251b3 · 90251b3
1 parent 7224245
commit 90251b3
Show file tree

Hide file tree

Showing 9 changed files with 120 additions and 11 deletions.
diff --git a/...ces/ca/uhn/hapi/fhir/changelog/7_4_0/6044-fix-issue-with-cache-refresh-infinite-loop.yaml b/...ces/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
@@ -151,6 +151,15 @@ public boolean canUseHibernateSearch(String theResourceType, SearchParameterMap
 			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/...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java b/...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java
@@ -89,6 +89,7 @@ public boolean supportsSearchParameter(String theParamName, ResourceSearchParams
 	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/...paserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java b/...paserver-searchparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCache.java
@@ -43,6 +43,9 @@
 @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 @@ protected boolean isTimeToRefresh() {
 		return myNextRefreshTime.isBefore(now());
 	}
 
-	private static Instant now() {
+	static Instant now() {
 		if (ourNowForUnitTests != null) {
 			return ourNowForUnitTests;
 		}
@@ -153,7 +156,7 @@ private ResourceChangeResult refreshCacheAndNotifyListenersWithRetry() {
 						return myResourceChangeListenerCacheRefresher.refreshCacheAndNotifyListener(this);
 					}
 				},
-				MAX_RETRIES);
+				getMaxRetries());
 		return refreshCacheRetrier.runWithRetry();
 	}
 
@@ -223,4 +226,8 @@ public String toString() {
 				.append("myInitialized", myInitialized)
 				.toString();
 	}
+
+	static int getMaxRetries() {
+		return MAX_RETRIES;
+	}
 }
diff --git a/...chparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java b/...chparam/src/main/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerCacheRefresherImpl.java
@@ -149,6 +149,7 @@ public ResourceChangeResult refreshCacheAndNotifyListener(IResourceChangeListene
 			return retVal;
 		}
 		SearchParameterMap searchParamMap = theCache.getSearchParameterMap();
+
 		ResourceVersionMap newResourceVersionMap =
 				myResourceVersionSvc.getVersionMap(theCache.getResourceName(), searchParamMap);
 

diff --git a/...archparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java b/...archparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java
@@ -132,6 +132,7 @@ public ResourceSearchParams getActiveSearchParams(String theResourceName) {
 
 	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 @@ private void initializeActiveSearchParams(Collection<IBaseResource> theJpaSearch
 			}
 		}
 
-		myActiveSearchParams = searchParams;
+		setActiveSearchParams(searchParams);
 
 		myJpaSearchParamCache.populateActiveSearchParams(
 				myInterceptorBroadcaster, myPhoneticEncoder, myActiveSearchParams);
@@ -432,9 +433,15 @@ public void handleInit(Collection<IIdType> theResourceIds) {
 		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 void setSearchParameterCanonicalizerForUnitTest(
 	public int getMaxManagedParamCountForUnitTests() {
 		return MAX_MANAGED_PARAM_COUNT;
 	}
+
+	@VisibleForTesting
+	public void setActiveSearchParams(RuntimeSearchParamCache theSearchParams) {
+		myActiveSearchParams = theSearchParams;
+	}
 }
diff --git a/...ver-test-r4/src/test/java/ca/uhn/fhir/jpa/cache/ResourceChangeListenerRegistryImplIT.java b/...ver-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,17 +18,24 @@
 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;
 
 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 void after() {
 		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<IIdType> 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<ResourceChangeListenerCache> 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/...aserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/...aserver-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 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
@@ -39,6 +39,16 @@
 // 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
 	 */