no-jira: hardened publisher select for update (at the cost of a query), better...

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)

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) {

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;
 		}