From 06529abf5aa7e55b304e4419972cc4de7a63f15a Mon Sep 17 00:00:00 2001 From: mhyeon-lee Date: Fri, 14 Feb 2020 18:18:51 +0900 Subject: [PATCH] DATAJDBC-488 - Avoid deadlocks by first accessing the root entity. The previous process of deleting referencing entities, updating the root and then inserting referencing entities hat the potential of causing deadlocks. When one process didn't obtain a lock on delete because there wasn't anything to delete root and referencing entities got locks in opposite order, which is a classical cause for deadlocks. Original pull request: #191. --- ...RepositoryConcurrencyIntegrationTests.java | 140 ++++++++++++++++++ ...itoryConcurrencyIntegrationTests-mysql.sql | 2 + .../core/conversion/WritingContext.java | 8 +- ...RelationalEntityUpdateWriterUnitTests.java | 5 +- .../RelationalEntityWriterUnitTests.java | 19 +-- 5 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryConcurrencyIntegrationTests.java create mode 100644 spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryConcurrencyIntegrationTests-mysql.sql diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryConcurrencyIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryConcurrencyIntegrationTests.java new file mode 100644 index 000000000..c1346a044 --- /dev/null +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryConcurrencyIntegrationTests.java @@ -0,0 +1,140 @@ +package org.springframework.data.jdbc.repository; + +import junit.framework.AssertionFailedError; +import lombok.AllArgsConstructor; +import lombok.Getter; +import lombok.With; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; +import org.springframework.data.annotation.Id; +import org.springframework.data.jdbc.repository.support.JdbcRepositoryFactory; +import org.springframework.data.jdbc.testing.TestConfiguration; +import org.springframework.data.repository.CrudRepository; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.rules.SpringClassRule; +import org.springframework.test.context.junit4.rules.SpringMethodRule; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.support.TransactionTemplate; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Myeonghyeon Lee + */ +@ContextConfiguration +@ActiveProfiles("mysql") +public class JdbcRepositoryConcurrencyIntegrationTests { + @Configuration + @Import(TestConfiguration.class) + static class Config { + + @Autowired JdbcRepositoryFactory factory; + + @Bean + Class testClass() { + return JdbcRepositoryConcurrencyIntegrationTests.class; + } + + @Bean + DummyEntityRepository dummyEntityRepository() { + return factory.getRepository(DummyEntityRepository.class); + } + } + + @ClassRule + public static final SpringClassRule classRule = new SpringClassRule(); + @Rule + public SpringMethodRule methodRule = new SpringMethodRule(); + + @Autowired + NamedParameterJdbcTemplate template; + @Autowired + DummyEntityRepository repository; + @Autowired + PlatformTransactionManager transactionManager; + + @Test // DATAJDBC-488 + public void updateConcurrencyWithEmptyReferences() throws Exception { + DummyEntity entity = createDummyEntity(); + entity = repository.save(entity); + + assertThat(entity.getId()).isNotNull(); + + List concurrencyEntities = new ArrayList<>(); + Element element1 = new Element(null, 1L); + Element element2 = new Element(null, 2L); + + for (int i = 0; i < 100; i++) { + List newContent = Arrays.asList( + element1.withContent(element1.content + i + 2), + element2.withContent(element2.content + i + 2) + ); + + concurrencyEntities.add(entity + .withName(entity.getName() + i) + .withContent(newContent)); + } + + TransactionTemplate transactionTemplate = new TransactionTemplate(this.transactionManager); + + List exceptions = new CopyOnWriteArrayList<>(); + CountDownLatch countDownLatch = new CountDownLatch(concurrencyEntities.size()); + concurrencyEntities.stream() + .map(e -> new Thread(() -> { + countDownLatch.countDown(); + try { + transactionTemplate.execute(status -> repository.save(e)); + } catch (Exception ex) { + exceptions.add(ex); + } + })) + .forEach(Thread::start); + + countDownLatch.await(); + + Thread.sleep(1000); + DummyEntity reloaded = repository.findById(entity.id).orElseThrow(AssertionFailedError::new); + assertThat(reloaded.content).hasSize(2); + assertThat(exceptions).isEmpty(); + } + + private static DummyEntity createDummyEntity() { + return new DummyEntity(null, "Entity Name", new ArrayList<>()); + } + + interface DummyEntityRepository extends CrudRepository { + } + + @Getter + @AllArgsConstructor + static class DummyEntity { + + @Id + private Long id; + @With + String name; + @With + final List content; + + } + + @AllArgsConstructor + static class Element { + + @Id private Long id; + @With final Long content; + } +} diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryConcurrencyIntegrationTests-mysql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryConcurrencyIntegrationTests-mysql.sql new file mode 100644 index 000000000..e0a8a767c --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryConcurrencyIntegrationTests-mysql.sql @@ -0,0 +1,2 @@ +CREATE TABLE dummy_entity ( id BIGINT AUTO_INCREMENT PRIMARY KEY, NAME VARCHAR(100)); +CREATE TABLE element (id BIGINT AUTO_INCREMENT PRIMARY KEY, content BIGINT, Dummy_Entity_key BIGINT,dummy_entity BIGINT); diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/WritingContext.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/WritingContext.java index 27cbcdf00..4d09aeacb 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/WritingContext.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/WritingContext.java @@ -37,6 +37,7 @@ import org.springframework.util.Assert; * @author Jens Schauder * @author Bastian Wilhelm * @author Mark Paluch + * @author Myeonghyeon Lee */ class WritingContext { @@ -73,14 +74,17 @@ class WritingContext { /** * Leaves out the isNew check as defined in #DATAJDBC-282 + * Possible Deadlocks in Execution Order in #DATAJDBC-488 * * @return List of {@link DbAction}s * @see DAJDBC-282 + * @see DAJDBC-488 */ List> update() { - List> actions = new ArrayList<>(deleteReferenced()); + List> actions = new ArrayList<>(); actions.add(setRootAction(new DbAction.UpdateRoot<>(entity))); + actions.addAll(deleteReferenced()); actions.addAll(insertReferenced()); return actions; } @@ -94,8 +98,8 @@ class WritingContext { actions.addAll(insertReferenced()); } else { - actions.addAll(deleteReferenced()); actions.add(setRootAction(new DbAction.UpdateRoot<>(entity))); + actions.addAll(deleteReferenced()); actions.addAll(insertReferenced()); } diff --git a/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityUpdateWriterUnitTests.java b/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityUpdateWriterUnitTests.java index 5ee9c31ed..1a80939fe 100644 --- a/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityUpdateWriterUnitTests.java +++ b/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityUpdateWriterUnitTests.java @@ -30,6 +30,7 @@ import org.springframework.data.relational.core.mapping.RelationalMappingContext * Unit tests for the {@link RelationalEntityUpdateWriter} * * @author Thomas Lang + * @author Myeonghyeon Lee */ @RunWith(MockitoJUnitRunner.class) public class RelationalEntityUpdateWriterUnitTests { @@ -51,8 +52,8 @@ public class RelationalEntityUpdateWriterUnitTests { .extracting(DbAction::getClass, DbAction::getEntityType, DbActionTestSupport::extractPath, DbActionTestSupport::actualEntityType, DbActionTestSupport::isWithDependsOn) // .containsExactly( // - tuple(DbAction.Delete.class, Element.class, "other", null, false), // - tuple(DbAction.UpdateRoot.class, SingleReferenceEntity.class, "", SingleReferenceEntity.class, false) // + tuple(DbAction.UpdateRoot.class, SingleReferenceEntity.class, "", SingleReferenceEntity.class, false), // + tuple(DbAction.Delete.class, Element.class, "other", null, false) // ); } diff --git a/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityWriterUnitTests.java b/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityWriterUnitTests.java index 1fa460a45..0fe9d7219 100644 --- a/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityWriterUnitTests.java +++ b/spring-data-relational/src/test/java/org/springframework/data/relational/core/conversion/RelationalEntityWriterUnitTests.java @@ -49,6 +49,7 @@ import org.springframework.lang.Nullable; * @author Jens Schauder * @author Bastian Wilhelm * @author Mark Paluch + * @author Myeonghyeon Lee */ @RunWith(MockitoJUnitRunner.class) public class RelationalEntityWriterUnitTests { @@ -156,9 +157,9 @@ public class RelationalEntityWriterUnitTests { DbActionTestSupport::extractPath, // DbActionTestSupport::actualEntityType, // DbActionTestSupport::isWithDependsOn) // - .containsExactly( // - tuple(Delete.class, Element.class, "other", null, false), // - tuple(UpdateRoot.class, SingleReferenceEntity.class, "", SingleReferenceEntity.class, false) // + .containsExactly( + tuple(UpdateRoot.class, SingleReferenceEntity.class, "", SingleReferenceEntity.class, false), // + tuple(Delete.class, Element.class, "other", null, false) // ); } @@ -180,8 +181,8 @@ public class RelationalEntityWriterUnitTests { DbActionTestSupport::actualEntityType, // DbActionTestSupport::isWithDependsOn) // .containsExactly( // - tuple(Delete.class, Element.class, "other", null, false), // tuple(UpdateRoot.class, SingleReferenceEntity.class, "", SingleReferenceEntity.class, false), // + tuple(Delete.class, Element.class, "other", null, false), // tuple(Insert.class, Element.class, "other", Element.class, true) // ); } @@ -291,9 +292,9 @@ public class RelationalEntityWriterUnitTests { DbActionTestSupport::actualEntityType, // DbActionTestSupport::isWithDependsOn) // .containsExactly( // + tuple(UpdateRoot.class, CascadingReferenceEntity.class, "", CascadingReferenceEntity.class, false), // tuple(Delete.class, Element.class, "other.element", null, false), tuple(Delete.class, CascadingReferenceMiddleElement.class, "other", null, false), - tuple(UpdateRoot.class, CascadingReferenceEntity.class, "", CascadingReferenceEntity.class, false), // tuple(Insert.class, CascadingReferenceMiddleElement.class, "other", CascadingReferenceMiddleElement.class, true), // tuple(Insert.class, CascadingReferenceMiddleElement.class, "other", CascadingReferenceMiddleElement.class, @@ -447,8 +448,8 @@ public class RelationalEntityWriterUnitTests { this::getMapKey, // DbActionTestSupport::extractPath) // .containsExactly( // - tuple(Delete.class, Element.class, null, "elements"), // tuple(UpdateRoot.class, MapContainer.class, null, ""), // + tuple(Delete.class, Element.class, null, "elements"), // tuple(Insert.class, Element.class, "one", "elements") // ); } @@ -469,8 +470,8 @@ public class RelationalEntityWriterUnitTests { this::getListKey, // DbActionTestSupport::extractPath) // .containsExactly( // - tuple(Delete.class, Element.class, null, "elements"), // tuple(UpdateRoot.class, ListContainer.class, null, ""), // + tuple(Delete.class, Element.class, null, "elements"), // tuple(Insert.class, Element.class, 0, "elements") // ); } @@ -494,9 +495,9 @@ public class RelationalEntityWriterUnitTests { a -> getQualifier(a, listMapContainerElements), // DbActionTestSupport::extractPath) // .containsExactly( // + tuple(UpdateRoot.class, ListMapContainer.class, null, null, ""), // tuple(Delete.class, Element.class, null, null, "maps.elements"), // tuple(Delete.class, MapContainer.class, null, null, "maps"), // - tuple(UpdateRoot.class, ListMapContainer.class, null, null, ""), // tuple(Insert.class, MapContainer.class, 0, null, "maps"), // tuple(Insert.class, Element.class, null, "one", "maps.elements") // ); @@ -521,9 +522,9 @@ public class RelationalEntityWriterUnitTests { a -> getQualifier(a, noIdListMapContainerElements), // DbActionTestSupport::extractPath) // .containsExactly( // + tuple(UpdateRoot.class, NoIdListMapContainer.class, null, null, ""), // tuple(Delete.class, NoIdElement.class, null, null, "maps.elements"), // tuple(Delete.class, NoIdMapContainer.class, null, null, "maps"), // - tuple(UpdateRoot.class, NoIdListMapContainer.class, null, null, ""), // tuple(Insert.class, NoIdMapContainer.class, 0, null, "maps"), // tuple(Insert.class, NoIdElement.class, 0, "one", "maps.elements") // );