From f70648484a59a79855922362873d31017499ec39 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 8 Apr 2021 12:28:42 -0400 Subject: [PATCH] Avoid a deadlock uploading terminology in postgres (#2529) * Fix #2493 * Try to fix intermittent test failure --- ...-avoid-deadlock-uploading-terminology.yaml | 5 +++ .../jpa/term/TermDeferredStorageSvcImpl.java | 6 ++- .../method/ConformanceMethodBindingTest.java | 41 +++++++++++++------ 3 files changed, 37 insertions(+), 15 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2493-avoid-deadlock-uploading-terminology.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2493-avoid-deadlock-uploading-terminology.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2493-avoid-deadlock-uploading-terminology.yaml new file mode 100644 index 00000000000..1672b9c204e --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2493-avoid-deadlock-uploading-terminology.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 2493 +title: "A database deadlock in Postgresql was observed when uploading large terminology CodeSystems using + deferred uploading. Thanks to Tyge Folke Nielsen for reporting and suggesting a fix!" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java index 1b9f9305109..6e36e2fb331 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java @@ -60,7 +60,9 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.Queue; import java.util.UUID; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; @@ -68,7 +70,7 @@ public class TermDeferredStorageSvcImpl implements ITermDeferredStorageSvc { private static final Logger ourLog = LoggerFactory.getLogger(TermDeferredStorageSvcImpl.class); final private List myDeferredCodeSystemsDeletions = Collections.synchronizedList(new ArrayList<>()); - final private List myDeferredCodeSystemVersionsDeletions = Collections.synchronizedList(new ArrayList<>()); + final private Queue myDeferredCodeSystemVersionsDeletions = new ConcurrentLinkedQueue<>(); final private List myDeferredConcepts = Collections.synchronizedList(new ArrayList<>()); final private List myDeferredValueSets = Collections.synchronizedList(new ArrayList<>()); final private List myDeferredConceptMaps = Collections.synchronizedList(new ArrayList<>()); @@ -498,7 +500,7 @@ public class TermDeferredStorageSvcImpl implements ITermDeferredStorageSvc { } @Override - public synchronized void deleteCodeSystemVersion(TermCodeSystemVersion theCodeSystemVersion) { + public void deleteCodeSystemVersion(TermCodeSystemVersion theCodeSystemVersion) { myDeferredCodeSystemVersionsDeletions.add(theCodeSystemVersion); } diff --git a/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/ConformanceMethodBindingTest.java b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/ConformanceMethodBindingTest.java index 2b3d8d09938..af3c956d1d8 100644 --- a/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/ConformanceMethodBindingTest.java +++ b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/ConformanceMethodBindingTest.java @@ -1,35 +1,50 @@ package ca.uhn.fhir.rest.server.method; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.FhirVersionEnum; import ca.uhn.fhir.rest.annotation.Metadata; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.IRestfulServer; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import com.google.common.collect.Lists; import org.hl7.fhir.instance.model.api.IBaseConformance; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Answers; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import javax.servlet.http.HttpServletRequest; import java.lang.reflect.Method; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.RETURNS_DEEP_STUBS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +@ExtendWith(MockitoExtension.class) public class ConformanceMethodBindingTest { - private FhirContext fhirContext; + @Mock(answer = Answers.RETURNS_DEEP_STUBS) + protected ServletRequestDetails mySrd; + @Mock(answer = Answers.RETURNS_DEEP_STUBS) + private FhirContext myFhirContext; private ConformanceMethodBinding conformanceMethodBinding; @BeforeEach public void setUp() { - fhirContext = mock(FhirContext.class); } private T init(T theCapabilityStatementProvider) throws NoSuchMethodException { T provider = spy(theCapabilityStatementProvider); Method method = provider.getClass().getDeclaredMethod("getServerConformance", HttpServletRequest.class, RequestDetails.class); - conformanceMethodBinding = new ConformanceMethodBinding(method, fhirContext, provider); + conformanceMethodBinding = new ConformanceMethodBinding(method, myFhirContext, provider); return provider; } @@ -37,9 +52,9 @@ public class ConformanceMethodBindingTest { public void invokeServerCached() throws NoSuchMethodException { TestResourceProvider provider = init(new TestResourceProvider()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); } @@ -47,12 +62,12 @@ public class ConformanceMethodBindingTest { public void invokeServerCacheExpires() throws NoSuchMethodException { TestResourceProviderSmallCache provider = init(new TestResourceProviderSmallCache()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); sleepAtLeast(20); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, timeout(10000).times(2)).getServerConformance(any(), any()); } @@ -61,10 +76,10 @@ public class ConformanceMethodBindingTest { public void invokeServerCacheDisabled() throws NoSuchMethodException { TestResourceProviderNoCache provider = init(new TestResourceProviderNoCache()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(2)).getServerConformance(any(), any()); } @@ -72,11 +87,11 @@ public class ConformanceMethodBindingTest { public void invokeServerCacheDisabledInSuperclass() throws NoSuchMethodException { TestResourceProviderNoCache2 provider = init(new TestResourceProviderNoCache2()); - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); // We currently don't scan the annotation on the superclass...Perhaps we should - conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mock(RequestDetails.class, RETURNS_DEEP_STUBS), new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); + conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), mySrd, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any()); } @@ -84,7 +99,7 @@ public class ConformanceMethodBindingTest { public void invokeServerNotCached_ClientControlled() throws NoSuchMethodException { TestResourceProvider provider = init(new TestResourceProvider()); - RequestDetails requestDetails = mock(RequestDetails.class, RETURNS_DEEP_STUBS); + RequestDetails requestDetails = mySrd; when(requestDetails.getHeaders(Constants.HEADER_CACHE_CONTROL)).thenReturn(Lists.newArrayList(Constants.CACHE_CONTROL_NO_CACHE)); conformanceMethodBinding.invokeServer(mock(IRestfulServer.class, RETURNS_DEEP_STUBS), requestDetails, new Object[]{mock(HttpServletRequest.class), mock(RequestDetails.class)}); verify(provider, times(1)).getServerConformance(any(), any());