From c1d06084b763712c9b645a339db4f263036436c6 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Tue, 4 Jul 2017 21:53:26 -0400 Subject: [PATCH 1/3] Avoid deadlock in JPA server when creating a large number of resources and using the RequestValidatingInterceptor --- .../jpa/dao/JpaValidationSupportDstu2.java | 8 + .../dao/dstu3/JpaValidationSupportDstu3.java | 15 +- .../uhn/fhir/jpa/config/TestDstu3Config.java | 1 + .../dstu3/BaseResourceProviderDstu3Test.java | 7 +- .../provider/dstu3/StressTestDstu3Test.java | 166 ++++++++++++++++++ src/changes/changes.xml | 5 + 6 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaValidationSupportDstu2.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaValidationSupportDstu2.java index 37060cf2bfe..8a592d6295c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaValidationSupportDstu2.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaValidationSupportDstu2.java @@ -1,5 +1,8 @@ package ca.uhn.fhir.jpa.dao; +import javax.transaction.Transactional; +import javax.transaction.Transactional.TxType; + /* * #%L * HAPI FHIR JPA Server @@ -31,6 +34,7 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.rest.param.UriParam; import ca.uhn.fhir.rest.server.IBundleProvider; +@Transactional(value=TxType.REQUIRED) public class JpaValidationSupportDstu2 implements IJpaValidationSupportDstu2 { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(JpaValidationSupportDstu2.class); @@ -52,11 +56,13 @@ public class JpaValidationSupportDstu2 implements IJpaValidationSupportDstu2 { private FhirContext myDstu2Ctx; @Override + @Transactional(value=TxType.SUPPORTS) public ValueSetExpansionComponent expandValueSet(FhirContext theCtx, ConceptSetComponent theInclude) { return null; } @Override + @Transactional(value=TxType.SUPPORTS) public ValueSet fetchCodeSystem(FhirContext theCtx, String theSystem) { return null; } @@ -95,11 +101,13 @@ public class JpaValidationSupportDstu2 implements IJpaValidationSupportDstu2 { } @Override + @Transactional(value=TxType.SUPPORTS) public boolean isCodeSystemSupported(FhirContext theCtx, String theSystem) { return false; } @Override + @Transactional(value=TxType.SUPPORTS) public CodeValidationResult validateCode(FhirContext theCtx, String theCodeSystem, String theCode, String theDisplay) { return null; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/JpaValidationSupportDstu3.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/JpaValidationSupportDstu3.java index 2e913f3a5cf..926b6b114c0 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/JpaValidationSupportDstu3.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/JpaValidationSupportDstu3.java @@ -3,11 +3,10 @@ package ca.uhn.fhir.jpa.dao.dstu3; import java.util.Collections; import java.util.List; -import org.hl7.fhir.dstu3.model.CodeSystem; -import org.hl7.fhir.dstu3.model.IdType; -import org.hl7.fhir.dstu3.model.Questionnaire; -import org.hl7.fhir.dstu3.model.StructureDefinition; -import org.hl7.fhir.dstu3.model.ValueSet; +import javax.transaction.Transactional; +import javax.transaction.Transactional.TxType; + +import org.hl7.fhir.dstu3.model.*; import org.hl7.fhir.dstu3.model.ValueSet.ConceptSetComponent; import org.hl7.fhir.dstu3.model.ValueSet.ValueSetExpansionComponent; import org.hl7.fhir.instance.model.api.IAnyResource; @@ -43,6 +42,7 @@ import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.param.UriParam; import ca.uhn.fhir.rest.server.IBundleProvider; +@Transactional(value=TxType.REQUIRED) public class JpaValidationSupportDstu3 implements IJpaValidationSupportDstu3 { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(JpaValidationSupportDstu3.class); @@ -72,6 +72,7 @@ public class JpaValidationSupportDstu3 implements IJpaValidationSupportDstu3 { @Override + @Transactional(value=TxType.SUPPORTS) public ValueSetExpansionComponent expandValueSet(FhirContext theCtx, ConceptSetComponent theInclude) { return null; } @@ -81,6 +82,7 @@ public class JpaValidationSupportDstu3 implements IJpaValidationSupportDstu3 { return fetchResource(theCtx, CodeSystem.class, theSystem); } + @SuppressWarnings("unchecked") @Override public T fetchResource(FhirContext theContext, Class theClass, String theUri) { IdType id = new IdType(theUri); @@ -143,11 +145,13 @@ public class JpaValidationSupportDstu3 implements IJpaValidationSupportDstu3 { } @Override + @Transactional(value=TxType.SUPPORTS) public boolean isCodeSystemSupported(FhirContext theCtx, String theSystem) { return false; } @Override + @Transactional(value=TxType.SUPPORTS) public CodeValidationResult validateCode(FhirContext theCtx, String theCodeSystem, String theCode, String theDisplay) { return null; } @@ -160,6 +164,7 @@ public class JpaValidationSupportDstu3 implements IJpaValidationSupportDstu3 { @Override + @Transactional(value=TxType.SUPPORTS) public List fetchAllStructureDefinitions(FhirContext theContext) { return Collections.emptyList(); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 89366320cb7..0a964f37767 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -37,6 +37,7 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { retVal.setUrl("jdbc:derby:memory:myUnitTestDB;create=true"); retVal.setUsername(""); retVal.setPassword(""); + retVal.setMaxTotal(4); DataSource dataSource = ProxyDataSourceBuilder .create(retVal) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/BaseResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/BaseResourceProviderDstu3Test.java index a5914cf0cf7..9ddf4692003 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/BaseResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/BaseResourceProviderDstu3Test.java @@ -31,6 +31,7 @@ import ca.uhn.fhir.jpa.config.dstu3.WebsocketDstu3Config; import ca.uhn.fhir.jpa.config.dstu3.WebsocketDstu3DispatcherConfig; import ca.uhn.fhir.jpa.dao.data.ISearchDao; import ca.uhn.fhir.jpa.dao.dstu3.BaseJpaDstu3Test; +import ca.uhn.fhir.jpa.dao.dstu3.SearchParamRegistryDstu3; import ca.uhn.fhir.jpa.interceptor.RestHookSubscriptionDstu3Interceptor; import ca.uhn.fhir.jpa.search.DatabaseBackedPagingProvider; import ca.uhn.fhir.jpa.search.ISearchCoordinatorSvc; @@ -47,7 +48,7 @@ import ca.uhn.fhir.util.TestUtil; public abstract class BaseResourceProviderDstu3Test extends BaseJpaDstu3Test { - private static JpaValidationSupportChainDstu3 myValidationSupport; + protected static JpaValidationSupportChainDstu3 myValidationSupport; protected static IGenericClient ourClient; protected static CloseableHttpClient ourHttpClient; protected static int ourPort; @@ -56,6 +57,7 @@ public abstract class BaseResourceProviderDstu3Test extends BaseJpaDstu3Test { protected static String ourServerBase; private static GenericWebApplicationContext ourWebApplicationContext; private TerminologyUploaderProviderDstu3 myTerminologyUploaderProvider; + protected static SearchParamRegistryDstu3 ourSearchParamRegistry; protected static DatabaseBackedPagingProvider ourPagingProvider; protected static RestHookSubscriptionDstu3Interceptor ourRestHookSubscriptionInterceptor; protected static ISearchDao mySearchEntityDao; @@ -150,7 +152,9 @@ public abstract class BaseResourceProviderDstu3Test extends BaseJpaDstu3Test { mySearchCoordinatorSvc = wac.getBean(ISearchCoordinatorSvc.class); mySearchEntityDao = wac.getBean(ISearchDao.class); ourRestHookSubscriptionInterceptor = wac.getBean(RestHookSubscriptionDstu3Interceptor.class); + ourSearchParamRegistry = wac.getBean(SearchParamRegistryDstu3.class); + myFhirCtx.getRestfulClientFactory().setSocketTimeout(5000000); ourClient = myFhirCtx.newRestfulGenericClient(ourServerBase); if (shouldLogClient()) { ourClient.registerInterceptor(new LoggingInterceptor(true)); @@ -159,6 +163,7 @@ public abstract class BaseResourceProviderDstu3Test extends BaseJpaDstu3Test { PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(5000, TimeUnit.MILLISECONDS); HttpClientBuilder builder = HttpClientBuilder.create(); builder.setConnectionManager(connectionManager); + builder.setMaxConnPerRoute(99); ourHttpClient = builder.build(); ourServer = server; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java new file mode 100644 index 00000000000..5b3b8a183c0 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java @@ -0,0 +1,166 @@ +package ca.uhn.fhir.jpa.provider.dstu3; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import java.util.List; +import java.util.UUID; + +import org.apache.commons.io.IOUtils; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidator; +import org.hl7.fhir.dstu3.model.*; +import org.hl7.fhir.dstu3.model.Bundle.BundleType; +import org.hl7.fhir.dstu3.model.Bundle.HTTPVerb; +import org.junit.*; + +import com.google.common.collect.Lists; + +import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; +import ca.uhn.fhir.util.TestUtil; + +public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { + + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(StressTestDstu3Test.class); + private RequestValidatingInterceptor myRequestValidatingInterceptor; + + @Before + public void before() throws Exception { + super.before(); + + myRequestValidatingInterceptor = new RequestValidatingInterceptor(); + FhirInstanceValidator module = new FhirInstanceValidator(); + module.setValidationSupport(myValidationSupport); + myRequestValidatingInterceptor.addValidatorModule(module); + } + + @After + public void after() throws Exception { + super.after(); + + ourRestServer.unregisterInterceptor(myRequestValidatingInterceptor); + } + + /** + * This test prevents a deadlock that was detected with a large number of + * threads creating resources and blocking on the searchparamcache refreshing + * (since this is a synchronized method) while the instance that was actually + * executing was waiting on a DB connection. This was solved by making + * JpaValidationSupportDstuXX be transactional, which it should have been + * anyhow. + */ + @Test + public void testMultithreadedSearchWithValidation() throws Exception { + ourRestServer.registerInterceptor(myRequestValidatingInterceptor); + + Bundle input = new Bundle(); + input.setType(BundleType.TRANSACTION); + for (int i = 0; i < 500; i++) { + Patient p = new Patient(); + p.addIdentifier().setSystem("http://test").setValue("BAR"); + input.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl("Patient"); + } + ourClient.transaction().withBundle(input).execute(); + + CloseableHttpResponse getMeta = ourHttpClient.execute(new HttpGet(ourServerBase + "/metadata")); + try { + assertEquals(200, getMeta.getStatusLine().getStatusCode()); + } finally { + IOUtils.closeQuietly(getMeta); + } + + List tasks = Lists.newArrayList(); + try { + for (int threadIndex = 0; threadIndex < 8; threadIndex++) { + SearchTask task = new SearchTask(); + tasks.add(task); + task.start(); + } + for (int threadIndex = 0; threadIndex < 8; threadIndex++) { + CreateTask task = new CreateTask(); + tasks.add(task); + task.start(); + } + } finally { + for (BaseTask next : tasks) { + next.join(); + } + } + + int total = 0; + for (BaseTask next : tasks) { + if (next.getError() != null) { + fail(next.getError().toString()); + } + total += next.getTaskCount(); + } + + ourLog.info("Loaded {} searches", total); + } + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } + + public class BaseTask extends Thread { + protected Throwable myError; + protected int myTaskCount = 0; + + public Throwable getError() { + return myError; + } + + public int getTaskCount() { + return myTaskCount; + } + + } + + private final class SearchTask extends BaseTask { + + @Override + public void run() { + CloseableHttpResponse get = null; + for (int i = 0; i < 20; i++) { + try { + get = ourHttpClient.execute(new HttpGet(ourServerBase + "/Patient?identifier=http%3A%2F%2Ftest%7CBAR," + UUID.randomUUID().toString())); + try { + assertEquals(200, get.getStatusLine().getStatusCode()); + myTaskCount++; + } finally { + IOUtils.closeQuietly(get); + } + } catch (Throwable e) { + ourLog.error("Failure during search", e); + myError = e; + return; + } + } + } + } + + private final class CreateTask extends BaseTask { + + @Override + public void run() { + for (int i = 0; i < 50; i++) { + try { + Patient p = new Patient(); + p.addIdentifier().setSystem("http://test").setValue("BAR").setType(new CodeableConcept().addCoding(new Coding().setSystem("http://foo").setCode("bar"))); + p.setGender(org.hl7.fhir.dstu3.model.Enumerations.AdministrativeGender.MALE); + ourClient.create().resource(p).execute(); + + ourSearchParamRegistry.forceRefresh(); + + } catch (Throwable e) { + ourLog.error("Failure during search", e); + myError = e; + return; + } + } + } + } + +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 8565c7fc406..a55ab24aa8e 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -120,6 +120,11 @@ JpaConformanceProvider now has a configuration setting to enable and disable adding resource counts to the server metadata. + + Avoid a deadlock in JPA server when the RequestValidatingInterceptor is being + used and a large number of resources are being created by clients at + the same time. + From d3fa442958307588d1203edfa7e2f8a036882911 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 5 Jul 2017 09:44:03 -0400 Subject: [PATCH 2/3] Skip stress tests on Travis --- hapi-fhir-jpaserver-base/pom.xml | 6 ++++++ .../src/test/java/ca/uhn/fhir/jpa/stresstest/README.txt | 5 +++++ .../dstu3 => stresstest}/StressTestDstu3Test.java | 7 ++++++- 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/README.txt rename hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/{provider/dstu3 => stresstest}/StressTestDstu3Test.java (96%) diff --git a/hapi-fhir-jpaserver-base/pom.xml b/hapi-fhir-jpaserver-base/pom.xml index 3488e8e3caa..df730aec92d 100644 --- a/hapi-fhir-jpaserver-base/pom.xml +++ b/hapi-fhir-jpaserver-base/pom.xml @@ -627,6 +627,9 @@ maven-surefire-plugin 1 + + **/stresstest/* + @@ -642,6 +645,9 @@ 2 true + + **/stresstest/* + diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/README.txt b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/README.txt new file mode 100644 index 00000000000..b931599a8c7 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/README.txt @@ -0,0 +1,5 @@ +Note: Tests in this package are not executed by Travis, because travis +just gets mad that the build is taking too long. So make sure to run them +locally! + +A normal build (mvn install) will run them. \ No newline at end of file diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java similarity index 96% rename from hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java rename to hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java index 5b3b8a183c0..b3f72096863 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/StressTestDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java @@ -1,4 +1,4 @@ -package ca.uhn.fhir.jpa.provider.dstu3; +package ca.uhn.fhir.jpa.stresstest; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; @@ -17,6 +17,7 @@ import org.junit.*; import com.google.common.collect.Lists; +import ca.uhn.fhir.jpa.provider.dstu3.BaseResourceProviderDstu3Test; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.util.TestUtil; @@ -108,6 +109,10 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { protected Throwable myError; protected int myTaskCount = 0; + public BaseTask() { + setDaemon(true); + } + public Throwable getError() { return myError; } From 6c47bd4c5111ca65ceebbbce63927e3c78da87f9 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 5 Jul 2017 20:25:06 -0400 Subject: [PATCH 3/3] Support transaction in testpage overlay --- .../src/main/webapp/WEB-INF/templates/home.html | 1 + .../src/main/webapp/WEB-INF/templates/result.html | 4 ++-- src/changes/changes.xml | 6 ++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/home.html b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/home.html index 46c3efc0523..f7aceb9872a 100644 --- a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/home.html +++ b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/home.html @@ -9,6 +9,7 @@
+
diff --git a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/result.html b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/result.html index 3db16f4cd74..597a25d01f2 100644 --- a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/result.html +++ b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/result.html @@ -315,8 +315,8 @@ - - + + diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a55ab24aa8e..55eab18f75d 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -125,6 +125,12 @@ used and a large number of resources are being created by clients at the same time. + + Testpage Overlay's transaction method did not work if the response + Bundle contained any entries that did not contain a resource (which + is often the case in more recent versions of HAPI). Thanks to Sujay R + for reporting! +