From af045033b094cfeb778b6658d8d0880632420cae Mon Sep 17 00:00:00 2001
From: srosse <none@none>
Date: Tue, 28 May 2013 16:28:35 +0200
Subject: [PATCH] no-jira: hardened publisher select for update (at the cost of
a query), better rollback suppport in DBImpl (possible lost rollback in a
hibernate session)
---
.../olat/core/commons/persistence/DBImpl.java | 66 ++++++++++---------
.../NotificationsManagerImpl.java | 52 +++++++--------
2 files changed, 57 insertions(+), 61 deletions(-)
diff --git a/src/main/java/org/olat/core/commons/persistence/DBImpl.java b/src/main/java/org/olat/core/commons/persistence/DBImpl.java
index e840fbbe9f0..34b80dd853a 100644
--- a/src/main/java/org/olat/core/commons/persistence/DBImpl.java
+++ b/src/main/java/org/olat/core/commons/persistence/DBImpl.java
@@ -36,6 +36,7 @@ import java.util.Map;
import javax.persistence.EntityManager;
import javax.persistence.EntityManagerFactory;
import javax.persistence.EntityTransaction;
+import javax.persistence.RollbackException;
import org.hibernate.HibernateException;
import org.hibernate.Query;
@@ -275,27 +276,7 @@ public class DBImpl extends LogDelegator implements DB, Destroyable {
logWarn("beginTransaction bulk-change, too many db access for one transaction, could be a performance problem (add closeSession/createSession in loop) logObject=" + logObject, null);
getData().resetAccessCounter();
}
-
- EntityTransaction transaction = em.getTransaction();
- if(transaction == null) {
- logError("Call beginTransaction but transaction is null", null);
- } else if (transaction.isActive() && transaction.getRollbackOnly()) {
- logError("Call beginTransaction but transaction is already rollbacked", null);
- } else if (!transaction.isActive()) {
- logError("Call beginTransaction but transaction is already completed", null);
- }
}
- /*
- private void createSession2() {
- ThreadLocalData data = getData();
- if (data == null) {
- if (isLogDebugEnabled()) logDebug("createSession start...", null);
- data.resetAccessCounter();
- data.resetCommitCounter();
- }
- setInitialized(true);
- if (isLogDebugEnabled()) logDebug("createSession end...", null);
- }*/
/**
* Close the database session.
@@ -317,15 +298,14 @@ public class DBImpl extends LogDelegator implements DB, Destroyable {
if(s != null) {
EntityTransaction trx = s.getTransaction();
if(trx.isActive()) {
- if(trx.getRollbackOnly()) {
+ try {
+ trx.commit();
+ } catch (RollbackException ex) {
+ //possible if trx setRollbackonly
+ logWarn("Close session with transaction set with setRollbackOnly", ex);
+ } catch (Exception e) {
+ logError("", e);
trx.rollback();
- } else {
- try {
- trx.commit();
- } catch (Exception e) {
- logError("", e);
- trx.rollback();
- }
}
}
@@ -778,10 +758,16 @@ public class DBImpl extends LogDelegator implements DB, Destroyable {
EntityTransaction trx = getCurrentEntityManager().getTransaction();
if(trx != null) {
+ logWarn("Commit commit", new Exception());
trx.commit();
}
if (isLogDebugEnabled()) logDebug("Commit DONE hasTransaction()=" + hasTransaction(), null);
+ } else if(hasTransaction() && isError()) {
+ EntityTransaction trx = getCurrentEntityManager().getTransaction();
+ if(trx != null && trx.isActive()) {
+ throw new DBRuntimeException("Try to commit a transaction in error status");
+ }
} else {
if (isLogDebugEnabled()) logDebug("Call commit without starting transaction", null );
}
@@ -800,8 +786,16 @@ public class DBImpl extends LogDelegator implements DB, Destroyable {
txManager.rollback(status);
EntityTransaction trx = getCurrentEntityManager().getTransaction();
- if(trx != null) {
- trx.rollback();
+ if(trx != null && trx.isActive()) {
+ if(trx.getRollbackOnly()) {
+ try {
+ trx.commit();
+ } catch (RollbackException e1) {
+ //we wait for this exception
+ }
+ } else {
+ trx.rollback();
+ }
}
}
} catch (Error er) {
@@ -829,8 +823,16 @@ public class DBImpl extends LogDelegator implements DB, Destroyable {
txManager.rollback(status);
EntityTransaction trx = getCurrentEntityManager().getTransaction();
- if(trx != null) {
- trx.rollback();
+ if(trx != null && trx.isActive()) {
+ if(trx.getRollbackOnly()) {
+ try {
+ trx.commit();
+ } catch (RollbackException e) {
+ //we wait for this exception
+ }
+ } else {
+ trx.rollback();
+ }
}
} catch (Exception ex) {
diff --git a/src/main/java/org/olat/notifications/NotificationsManagerImpl.java b/src/main/java/org/olat/notifications/NotificationsManagerImpl.java
index 983bfd843e3..dbc6286cb50 100644
--- a/src/main/java/org/olat/notifications/NotificationsManagerImpl.java
+++ b/src/main/java/org/olat/notifications/NotificationsManagerImpl.java
@@ -591,30 +591,10 @@ public class NotificationsManagerImpl extends NotificationsManager implements Us
*/
@Override
public Publisher getPublisher(SubscriptionContext subsContext) {
- return getPublisher(subsContext.getResName(), subsContext.getResId(), subsContext.getSubidentifier(), false);
- }
-
- private Publisher getPublisherForUpdate(SubscriptionContext subsContext) {
- return getPublisher(subsContext.getResName(), subsContext.getResId(), subsContext.getSubidentifier(), true);
- }
-
- @Override
- public List<Publisher> getAllPublisher() {
- String q = "select pub from org.olat.notifications.PublisherImpl pub";
- return DBFactory.getInstance().getCurrentEntityManager().createQuery(q, Publisher.class)
- .getResultList();
- }
-
- /**
- * return the publisher for the given composite primary key ores +
- * subidentifier.
- */
- private Publisher getPublisher(String resName, Long resId, String subidentifier, boolean forUpdate) {
-
StringBuilder q = new StringBuilder();
q.append("select pub from ").append(PublisherImpl.class.getName()).append(" pub ")
.append(" where pub.resName=:resName and pub.resId = :resId");
- if(StringHelper.containsNonWhitespace(subidentifier)) {
+ if(StringHelper.containsNonWhitespace(subsContext.getSubidentifier())) {
q.append(" and pub.subidentifier=:subidentifier");
} else {
q.append(" and (pub.subidentifier='' or pub.subidentifier is null)");
@@ -622,20 +602,34 @@ public class NotificationsManagerImpl extends NotificationsManager implements Us
TypedQuery<Publisher> query = DBFactory.getInstance().getCurrentEntityManager()
.createQuery(q.toString(), Publisher.class)
- .setParameter("resName", resName)
- .setParameter("resId", resId.longValue());
+ .setParameter("resName", subsContext.getResName())
+ .setParameter("resId", subsContext.getResId());
- if(StringHelper.containsNonWhitespace(subidentifier)) {
- query.setParameter("subidentifier", subidentifier);
- }
- if(forUpdate) {
- query.setLockMode(LockModeType.PESSIMISTIC_WRITE);
+ if(StringHelper.containsNonWhitespace(subsContext.getSubidentifier())) {
+ query.setParameter("subidentifier", subsContext.getSubidentifier());
}
List<Publisher> res = query.getResultList();
if (res.isEmpty()) return null;
if (res.size() != 1) throw new AssertException("only one subscriber per person and publisher!!");
return res.get(0);
}
+
+ private Publisher getPublisherForUpdate(SubscriptionContext subsContext) {
+ Publisher pub = getPublisher(subsContext);
+ if(pub != null && pub.getKey() != null) {
+ //prevent optimistic lock issue
+ DBFactory.getInstance().getCurrentEntityManager().detach(pub);
+ pub = DBFactory.getInstance().getCurrentEntityManager().find(PublisherImpl.class, pub.getKey(), LockModeType.PESSIMISTIC_WRITE);
+ }
+ return pub;
+ }
+
+ @Override
+ public List<Publisher> getAllPublisher() {
+ String q = "select pub from org.olat.notifications.PublisherImpl pub";
+ return DBFactory.getInstance().getCurrentEntityManager().createQuery(q, Publisher.class)
+ .getResultList();
+ }
/**
* @param resName
@@ -843,7 +837,7 @@ public class NotificationsManagerImpl extends NotificationsManager implements Us
// to make sure: ignore if no subscriptionContext
if (subscriptionContext == null) return;
- Publisher toUpdate = getPublisher(subscriptionContext.getResName(), subscriptionContext.getResId(), subscriptionContext.getSubidentifier(), true);
+ Publisher toUpdate = getPublisherForUpdate(subscriptionContext);
if(toUpdate == null) {
return;
}
--
GitLab