From 29d8acf4d5723f5919d95c507871a7322361537d Mon Sep 17 00:00:00 2001
From: Andreas Svanberg <andreass@dsv.su.se>
Date: Mon, 22 Apr 2024 15:28:08 +0200
Subject: [PATCH] 3361 Fix simultaneous requests to open the same checklist
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.
---
.../activityplan/ActivityPlanFacadeImpl.java | 28 +++++++++++++++++--
.../scipro/checklist/ChecklistService.java | 5 +++-
.../checklist/ChecklistServiceImpl.java | 16 +++++++++++
3 files changed, 45 insertions(+), 4 deletions(-)
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