From c6777578a8b96eb5d9ea38bd280c40b8ca527e62 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Tue, 1 Sep 2020 09:52:38 -0400 Subject: [PATCH] Avoid DB binary storage deadlock (#2062) * Avoid DB binary storage deadlock * Add changelog * Rework --- .../2062-avoid-db-bin-storage-deadlock.yaml | 5 +++ .../ca/uhn/fhir/jpa/api/config/DaoConfig.java | 28 ++------------ .../DatabaseBlobBinaryStorageSvcImpl.java | 37 ++++++++----------- .../jpa/dao/data/IBinaryStorageEntityDao.java | 8 ---- .../DatabaseBlobBinaryStorageSvcImplTest.java | 16 +------- .../jpa/model/entity/BinaryStorageEntity.java | 8 ++++ 6 files changed, 33 insertions(+), 69 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2062-avoid-db-bin-storage-deadlock.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2062-avoid-db-bin-storage-deadlock.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2062-avoid-db-bin-storage-deadlock.yaml new file mode 100644 index 00000000000..a767efaa4f9 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2062-avoid-db-bin-storage-deadlock.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 2062 +title: "A deadlock was fixed where the Database-backed Binary Storage service could lock the system up when running + under very high contention." diff --git a/hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java b/hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java index 98f49f6ff2d..03c8f3f5c5b 100644 --- a/hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java +++ b/hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java @@ -222,11 +222,6 @@ public class DaoConfig { */ private boolean myLastNEnabled = false; - /** - * @since 5.1.0 - */ - private boolean myPreloadBlobFromInputStream = false; - /** * Constructor */ @@ -2102,28 +2097,11 @@ public class DaoConfig { *

* * @since 5.1.0 + * @deprecated In 5.2.0 this setting no longer does anything */ - public boolean isPreloadBlobFromInputStream() { - return myPreloadBlobFromInputStream; - } - - /** - *

- * This determines whether $binary-access-write operations should first load the InputStream into memory before persisting the - * contents to the database. This needs to be enabled for MS SQL Server as this DB requires that the blob size be known - * in advance. - *

- *

- * Note that this setting should be enabled with caution as it can lead to significant demands on memory. - *

- *

- * The default value for this setting is {@code false}. - *

- * - * @since 5.1.0 - */ + @Deprecated public void setPreloadBlobFromInputStream(Boolean thePreloadBlobFromInputStream) { - myPreloadBlobFromInputStream = thePreloadBlobFromInputStream; + // ignore } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java index 54a9e0e023c..1c5dd64ce9c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java @@ -60,8 +60,15 @@ public class DatabaseBlobBinaryStorageSvcImpl extends BaseBinaryStorageSvcImpl { private DaoConfig myDaoConfig; @Override - @Transactional(Transactional.TxType.SUPPORTS) + @Transactional(Transactional.TxType.REQUIRED) public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, String theContentType, InputStream theInputStream) throws IOException { + + /* + * Note on transactionality: This method used to have a propagation value of SUPPORTS and then do the actual + * write in a new transaction.. I don't actually get why that was the original design, but it causes + * connection pool deadlocks under load! + */ + Date publishedDate = new Date(); HashingInputStream hashingInputStream = createHashingInputStream(theInputStream); @@ -77,32 +84,18 @@ public class DatabaseBlobBinaryStorageSvcImpl extends BaseBinaryStorageSvcImpl { Session session = (Session) myEntityManager.getDelegate(); LobHelper lobHelper = session.getLobHelper(); - Blob dataBlob; - if (myDaoConfig.isPreloadBlobFromInputStream()) { - byte[] loadedStream = IOUtils.toByteArray(countingInputStream); - dataBlob = lobHelper.createBlob(loadedStream); - } else { - dataBlob = lobHelper.createBlob(countingInputStream, 0); - } + byte[] loadedStream = IOUtils.toByteArray(countingInputStream); + Blob dataBlob = lobHelper.createBlob(loadedStream); entity.setBlob(dataBlob); - // Save the entity - - TransactionTemplate txTemplate = new TransactionTemplate(myPlatformTransactionManager); - txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); - txTemplate.execute(t -> { - myEntityManager.persist(entity); - return null; - }); - // Update the entity with the final byte count and hash long bytes = countingInputStream.getCount(); String hash = hashingInputStream.hash().toString(); - txTemplate.execute(t -> { - myBinaryStorageEntityDao.setSize(id, (int) bytes); - myBinaryStorageEntityDao.setHash(id, hash); - return null; - }); + entity.setSize((int) bytes); + entity.setHash(hash); + + // Save the entity + myEntityManager.persist(entity); return new StoredDetails() .setBlobId(id) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBinaryStorageEntityDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBinaryStorageEntityDao.java index e32a3604a66..dc17d8c1429 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBinaryStorageEntityDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBinaryStorageEntityDao.java @@ -30,14 +30,6 @@ import java.util.Optional; public interface IBinaryStorageEntityDao extends JpaRepository { - @Modifying - @Query("UPDATE BinaryStorageEntity e SET e.mySize = :blob_size WHERE e.myBlobId = :blob_id") - void setSize(@Param("blob_id") String theId, @Param("blob_size") int theBytes); - - @Modifying - @Query("UPDATE BinaryStorageEntity e SET e.myHash = :blob_hash WHERE e.myBlobId = :blob_id") - void setHash(@Param("blob_id") String theId, @Param("blob_hash") String theHash); - @Query("SELECT e FROM BinaryStorageEntity e WHERE e.myBlobId = :blob_id AND e.myResourceId = :resource_id") Optional findByIdAndResourceId(@Param("blob_id") String theBlobId, @Param("resource_id") String theResourceId); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java index c97eec91e5b..3c4bddd735b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java @@ -45,18 +45,6 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test { @Autowired private DaoConfig myDaoConfig; - @BeforeEach - public void backupDaoConfig() { - defaultPreloadBlobFromInputStream = myDaoConfig.isPreloadBlobFromInputStream(); - } - - @AfterEach - public void restoreDaoConfig() { - myDaoConfig.setPreloadBlobFromInputStream(defaultPreloadBlobFromInputStream); - } - - boolean defaultPreloadBlobFromInputStream; - @Test public void testStoreAndRetrieve() throws IOException { @@ -74,7 +62,7 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test { assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size()); assertEquals(1, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size()); - assertEquals(2, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size()); + assertEquals(0, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size()); myCaptureQueriesListener.clear(); @@ -128,7 +116,7 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test { assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size()); assertEquals(1, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size()); - assertEquals(2, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size()); + assertEquals(0, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size()); myCaptureQueriesListener.clear(); diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BinaryStorageEntity.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BinaryStorageEntity.java index 3fe3308b0b0..c496232c52e 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BinaryStorageEntity.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BinaryStorageEntity.java @@ -89,4 +89,12 @@ public class BinaryStorageEntity { public String getBlobId() { return myBlobId; } + + public void setSize(int theSize) { + mySize = theSize; + } + + public void setHash(String theHash) { + myHash = theHash; + } }