From 47867fc628ebe881fe9ceb72f3e7e6ca14ef359a Mon Sep 17 00:00:00 2001 From: Etienne Poirier <33007955+epeartree@users.noreply.github.com> Date: Wed, 24 Jan 2024 07:40:21 -0500 Subject: [PATCH] 5621 deadlock on caffeine cache when creating a resource with conditional create (#5622) * Modifying the CacheProvider to avoid doing db I/O within the cache miss value supplier callback. * Setting the initial capacity of instantiated caches to the cache max size to avoid resizing during operations. * adding changelog and spotless. * Fixing typo. * Addressing comments from code review. --------- Co-authored-by: peartree --- ...-on-caffeine-cash-on-conditional-create.yaml | 4 ++++ .../uhn/fhir/jpa/dao/index/IdHelperService.java | 17 +++++++++++++---- .../fhir/sl/cache/caffeine/CacheProvider.java | 6 ++++++ 3 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5621-fix-potential-deadlock-on-caffeine-cash-on-conditional-create.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5621-fix-potential-deadlock-on-caffeine-cash-on-conditional-create.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5621-fix-potential-deadlock-on-caffeine-cash-on-conditional-create.yaml new file mode 100644 index 00000000000..5fb4b901ffe --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5621-fix-potential-deadlock-on-caffeine-cash-on-conditional-create.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 5621 +title: "Fixed a deadlock in resource conditional create." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index b21c64dba93..93c0de299e3 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -477,10 +477,19 @@ public class IdHelperService implements IIdHelperService { @Override public Optional translatePidIdToForcedIdWithCache(JpaPid theId) { - return myMemoryCacheService.get( - MemoryCacheService.CacheEnum.PID_TO_FORCED_ID, - theId.getId(), - pid -> myResourceTableDao.findById(pid).map(ResourceTable::asTypedFhirResourceId)); + // do getIfPresent and then put to avoid doing I/O inside the cache. + Optional forcedId = + myMemoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.PID_TO_FORCED_ID, theId.getId()); + + if (forcedId == null) { + // This is only called when we know the resource exists. + // So this optional is only empty when there is no hfj_forced_id table + // note: this is obsolete with the new fhir_id column, and will go away. + forcedId = myResourceTableDao.findById(theId.getId()).map(ResourceTable::asTypedFhirResourceId); + myMemoryCacheService.put(MemoryCacheService.CacheEnum.PID_TO_FORCED_ID, theId.getId(), forcedId); + } + + return forcedId; } private ListMultimap organizeIdsByResourceType(Collection theIds) { diff --git a/hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine/src/main/java/ca/uhn/fhir/sl/cache/caffeine/CacheProvider.java b/hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine/src/main/java/ca/uhn/fhir/sl/cache/caffeine/CacheProvider.java index 6a1376410f0..cc324964f02 100644 --- a/hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine/src/main/java/ca/uhn/fhir/sl/cache/caffeine/CacheProvider.java +++ b/hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine/src/main/java/ca/uhn/fhir/sl/cache/caffeine/CacheProvider.java @@ -44,6 +44,9 @@ public class CacheProvider implements ca.uhn.fhir.sl.cache.CacheProvider create(long timeoutMillis, long maximumSize) { return new CacheDelegator(Caffeine.newBuilder() .expireAfterWrite(timeoutMillis, TimeUnit.MILLISECONDS) + // Caffeine locks the whole array when growing the hash table. + // Set initial capacity to max to avoid this. All our caches are <1M entries. + .initialCapacity((int) maximumSize) .maximumSize(maximumSize) .build()); } @@ -51,6 +54,9 @@ public class CacheProvider implements ca.uhn.fhir.sl.cache.CacheProvider create(long timeoutMillis, long maximumSize, CacheLoader loading) { return new LoadingCacheDelegator(Caffeine.newBuilder() .expireAfterWrite(timeoutMillis, TimeUnit.MILLISECONDS) + // Caffeine locks the whole array when growing the hash table. + // Set initial capacity to max to avoid this. All our caches are <1M entries. + .initialCapacity((int) maximumSize) .maximumSize(maximumSize) .build(loading::load)); }