diff --git a/core/src/main/java/se/su/dsv/scipro/activityplan/ActivityPlanFacadeImpl.java b/core/src/main/java/se/su/dsv/scipro/activityplan/ActivityPlanFacadeImpl.java index 89281bcc15..2f76f0fc8c 100755 --- a/core/src/main/java/se/su/dsv/scipro/activityplan/ActivityPlanFacadeImpl.java +++ b/core/src/main/java/se/su/dsv/scipro/activityplan/ActivityPlanFacadeImpl.java @@ -25,6 +25,7 @@ import se.su.dsv.scipro.system.User; import javax.inject.Inject; import java.util.*; +import java.util.concurrent.locks.ReentrantLock; import static com.querydsl.core.types.dsl.Expressions.allOf; @@ -312,11 +313,32 @@ public class ActivityPlanFacadeImpl implements ActivityPlanFacade { return checklistService.save(checklist); } + private static final ReentrantLock LOCK = new ReentrantLock(); + @Override - @Transactional public Checklist updateUserLastOpenDate(Checklist checklist, User user) { - checklist.getUserLastOpenDate().put(user, new Date()); - return checklistService.save(checklist); + // SCIPRO-3361 + // What happens in there are simultaneous requests to open a checklist. + // Both threads will read the checklist from the database, and since it + // is versioned they will both get version N. + // Then both threads will try to update the last open date and save the + // checklist. When saving a versioned object a SQL query of the form + // UPDATE checklist SET version = N+1 WHERE id = ? AND version = N + // is generated, the first such query will succeed but the second one + // will not update any row which Hibernate treats as an optimistic + // locking error. + // To work around this we use a global lock to ensure that only one + // thread can update the checklist at a time. We can't use the + // EntityManagers lock method since that too will cause an optimistic + // locking failure. This is an unfortunate global lock that will affect + // everyone even if they open different checklist, but it is such an + // underused feature that it should not be a problem. + try { + LOCK.lock(); + return checklistService.updateUserLastOpenDate(checklist, user); + } finally { + LOCK.unlock(); + } } @Override diff --git a/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistService.java b/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistService.java index 2613faffc2..30cc7e1806 100755 --- a/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistService.java +++ b/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistService.java @@ -2,10 +2,13 @@ package se.su.dsv.scipro.checklist; import se.su.dsv.scipro.project.Project; import se.su.dsv.scipro.system.GenericService; +import se.su.dsv.scipro.system.User; public interface ChecklistService extends GenericService<Checklist, Long> { Long countAnswers(Project project, ChecklistAnswerEnum answer); Long countUnanswered(Project project); - } + + Checklist updateUserLastOpenDate(Checklist checklist, User user); +} diff --git a/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistServiceImpl.java b/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistServiceImpl.java index 7274655864..d0b6640c60 100755 --- a/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistServiceImpl.java +++ b/core/src/main/java/se/su/dsv/scipro/checklist/ChecklistServiceImpl.java @@ -1,5 +1,6 @@ package se.su.dsv.scipro.checklist; +import com.google.inject.persist.Transactional; import se.su.dsv.scipro.activityplan.QActivity; import se.su.dsv.scipro.activityplan.QActivityPlan; import se.su.dsv.scipro.project.Project; @@ -8,6 +9,9 @@ import se.su.dsv.scipro.system.AbstractServiceImpl; import javax.inject.Inject; import javax.inject.Provider; import jakarta.persistence.EntityManager; +import se.su.dsv.scipro.system.User; + +import java.util.Date; public class ChecklistServiceImpl extends AbstractServiceImpl<Checklist, Long> implements ChecklistService { @Inject @@ -48,4 +52,16 @@ public class ChecklistServiceImpl extends AbstractServiceImpl<Checklist, Long> i return questions - answers; } + + @Override + @Transactional + public Checklist updateUserLastOpenDate(Checklist checklist, User user) { + EntityManager em = em(); + // SCIPRO-3361 + // Refresh checklist to prevent optimistic locking update errors + // if there are multiple threads updating the same checklist + em.refresh(checklist); + checklist.getUserLastOpenDate().put(user, new Date()); + return save(checklist); + } } \ No newline at end of file