HHH-3543: using BeforeTransactionCompletionProcess instead of a transaction synchronization. That way exceptions are not swallowed if one is thrown in the audit synchronization/process, and the tx is rolled back

git-svn-id: https://svn.jboss.org/repos/hibernate/core/trunk@19180 1b8cb986-b30d-0410-93ca-fae66ebed9b2

envers/src/main/java/org/hibernate/envers/configuration/AuditConfiguration.java

@@ -30,7 +30,7 @@ import java.util.WeakHashMap;
 import org.hibernate.envers.entities.EntitiesConfigurations;
 import org.hibernate.envers.revisioninfo.RevisionInfoNumberReader;
 import org.hibernate.envers.revisioninfo.RevisionInfoQueryCreator;
-import org.hibernate.envers.synchronization.AuditSyncManager;
+import org.hibernate.envers.synchronization.AuditProcessManager;
 
 import org.hibernate.cfg.Configuration;
 import org.hibernate.cfg.AnnotationConfiguration;
@@ -42,7 +42,7 @@ import org.hibernate.annotations.common.reflection.ReflectionManager;
 public class AuditConfiguration {
     private final GlobalConfiguration globalCfg;
     private final AuditEntitiesConfiguration auditEntCfg;
-    private final AuditSyncManager auditSyncManager;
+    private final AuditProcessManager auditProcessManager;
     private final EntitiesConfigurations entCfg;
     private final RevisionInfoQueryCreator revisionInfoQueryCreator;
     private final RevisionInfoNumberReader revisionInfoNumberReader;
@@ -51,8 +51,8 @@ public class AuditConfiguration {
         return auditEntCfg;
     }
 
-    public AuditSyncManager getSyncManager() {
+    public AuditProcessManager getSyncManager() {
-        return auditSyncManager;
+        return auditProcessManager;
     }
 
     public GlobalConfiguration getGlobalCfg() {
@@ -80,7 +80,7 @@ public class AuditConfiguration {
         RevisionInfoConfigurationResult revInfoCfgResult = revInfoCfg.configure(cfg, reflectionManager);
         auditEntCfg = new AuditEntitiesConfiguration(properties, revInfoCfgResult.getRevisionInfoEntityName());
         globalCfg = new GlobalConfiguration(properties);
-        auditSyncManager = new AuditSyncManager(revInfoCfgResult.getRevisionInfoGenerator());
+        auditProcessManager = new AuditProcessManager(revInfoCfgResult.getRevisionInfoGenerator());
         revisionInfoQueryCreator = revInfoCfgResult.getRevisionInfoQueryCreator();
         revisionInfoNumberReader = revInfoCfgResult.getRevisionInfoNumberReader();
         entCfg = new EntitiesConfigurator().configure(cfg, reflectionManager, globalCfg, auditEntCfg,

envers/src/main/java/org/hibernate/envers/event/AuditEventListener.java

@@ -31,7 +31,7 @@ import org.hibernate.envers.entities.RelationDescription;
 import org.hibernate.envers.entities.RelationType;
 import org.hibernate.envers.entities.mapper.PersistentCollectionChangeData;
 import org.hibernate.envers.entities.mapper.id.IdMapper;
-import org.hibernate.envers.synchronization.AuditSync;
+import org.hibernate.envers.synchronization.AuditProcess;
 import org.hibernate.envers.synchronization.work.*;
 import org.hibernate.envers.tools.Tools;
 import org.hibernate.envers.RevisionType;
@@ -68,7 +68,7 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
 
     private AuditConfiguration verCfg;
 
-    private void generateBidirectionalCollectionChangeWorkUnits(AuditSync verSync, EntityPersister entityPersister,
+    private void generateBidirectionalCollectionChangeWorkUnits(AuditProcess auditProcess, EntityPersister entityPersister,
                                                                 String entityName, Object[] newState, Object[] oldState,
                                                                 SessionImplementor session) {
         // Checking if this is enabled in configuration ...
@@ -112,7 +112,7 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
                          	id = (Serializable) idMapper.mapToIdFromEntity(newValue);
                     	}
 
-                        verSync.addWorkUnit(new CollectionChangeWorkUnit(session, toEntityName, verCfg, id, newValue));
+                        auditProcess.addWorkUnit(new CollectionChangeWorkUnit(session, toEntityName, verCfg, id, newValue));
                     }
 
                     if (oldValue != null) {
@@ -132,7 +132,7 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
 							id = (Serializable) idMapper.mapToIdFromEntity(oldValue);
                     	}
 						
-                        verSync.addWorkUnit(new CollectionChangeWorkUnit(session, toEntityName, verCfg, id, oldValue));
+                        auditProcess.addWorkUnit(new CollectionChangeWorkUnit(session, toEntityName, verCfg, id, oldValue));
                     }
                 }
             }
@@ -143,14 +143,14 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
         String entityName = event.getPersister().getEntityName();
 
         if (verCfg.getEntCfg().isVersioned(entityName)) {
-            AuditSync verSync = verCfg.getSyncManager().get(event.getSession());
+            AuditProcess auditProcess = verCfg.getSyncManager().get(event.getSession());
 
             AuditWorkUnit workUnit = new AddWorkUnit(event.getSession(), event.getPersister().getEntityName(), verCfg,
                     event.getId(), event.getPersister(), event.getState());
-            verSync.addWorkUnit(workUnit);
+            auditProcess.addWorkUnit(workUnit);
 
             if (workUnit.containsWork()) {
-                generateBidirectionalCollectionChangeWorkUnits(verSync, event.getPersister(), entityName, event.getState(),
+                generateBidirectionalCollectionChangeWorkUnits(auditProcess, event.getPersister(), entityName, event.getState(),
                         null, event.getSession());
             }
         }
@@ -160,14 +160,14 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
         String entityName = event.getPersister().getEntityName();
 
         if (verCfg.getEntCfg().isVersioned(entityName)) {
-            AuditSync verSync = verCfg.getSyncManager().get(event.getSession());
+            AuditProcess auditProcess = verCfg.getSyncManager().get(event.getSession());
 
             AuditWorkUnit workUnit = new ModWorkUnit(event.getSession(), event.getPersister().getEntityName(), verCfg,
                     event.getId(), event.getPersister(), event.getState(), event.getOldState());
-            verSync.addWorkUnit(workUnit);
+            auditProcess.addWorkUnit(workUnit);
 
             if (workUnit.containsWork()) {
-                generateBidirectionalCollectionChangeWorkUnits(verSync, event.getPersister(), entityName, event.getState(),
+                generateBidirectionalCollectionChangeWorkUnits(auditProcess, event.getPersister(), entityName, event.getState(),
                         event.getOldState(), event.getSession());
             }
         }
@@ -177,20 +177,20 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
         String entityName = event.getPersister().getEntityName();
 
         if (verCfg.getEntCfg().isVersioned(entityName)) {
-            AuditSync verSync = verCfg.getSyncManager().get(event.getSession());
+            AuditProcess auditProcess = verCfg.getSyncManager().get(event.getSession());
 
             AuditWorkUnit workUnit = new DelWorkUnit(event.getSession(), event.getPersister().getEntityName(), verCfg,
                     event.getId(), event.getPersister(), event.getDeletedState());
-            verSync.addWorkUnit(workUnit);
+            auditProcess.addWorkUnit(workUnit);
 
             if (workUnit.containsWork()) {
-                generateBidirectionalCollectionChangeWorkUnits(verSync, event.getPersister(), entityName, null,
+                generateBidirectionalCollectionChangeWorkUnits(auditProcess, event.getPersister(), entityName, null,
                         event.getDeletedState(), event.getSession());
             }
         }
     }
 
-    private void generateBidirectionalCollectionChangeWorkUnits(AuditSync verSync, AbstractCollectionEvent event,
+    private void generateBidirectionalCollectionChangeWorkUnits(AuditProcess auditProcess, AbstractCollectionEvent event,
                                                                 PersistentCollectionChangeWorkUnit workUnit,
                                                                 RelationDescription rd) {
         // Checking if this is enabled in configuration ...
@@ -209,13 +209,13 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
                 Object relatedObj = changeData.getChangedElement();
                 Serializable relatedId = (Serializable) relatedIdMapper.mapToIdFromEntity(relatedObj);
 
-                verSync.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), relatedEntityName, verCfg,
+                auditProcess.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), relatedEntityName, verCfg,
 						relatedId, relatedObj));
             }
         }
     }
 
-    private void generateFakeBidirecationalRelationWorkUnits(AuditSync verSync, PersistentCollection newColl, Serializable oldColl,
+    private void generateFakeBidirecationalRelationWorkUnits(AuditProcess auditProcess, PersistentCollection newColl, Serializable oldColl,
                                                              String collectionEntityName, String referencingPropertyName,
                                                              AbstractCollectionEvent event,
                                                              RelationDescription rd) {
@@ -242,13 +242,13 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
             AuditWorkUnit nestedWorkUnit = new CollectionChangeWorkUnit(event.getSession(), realRelatedEntityName, verCfg,
                     relatedId, relatedObj);
 
-            verSync.addWorkUnit(new FakeBidirectionalRelationWorkUnit(event.getSession(), realRelatedEntityName, verCfg,
+            auditProcess.addWorkUnit(new FakeBidirectionalRelationWorkUnit(event.getSession(), realRelatedEntityName, verCfg,
                     relatedId, referencingPropertyName, event.getAffectedOwnerOrNull(), rd, revType,
                     changeData.getChangedElementIndex(), nestedWorkUnit));
         }
 
         // We also have to generate a collection change work unit for the owning entity.
-        verSync.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), collectionEntityName, verCfg,
+        auditProcess.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), collectionEntityName, verCfg,
                 event.getAffectedOwnerIdOrNull(), event.getAffectedOwnerOrNull()));
     }
 
@@ -257,7 +257,7 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
         String entityName = event.getAffectedOwnerEntityName();
 
         if (verCfg.getEntCfg().isVersioned(entityName)) {
-            AuditSync verSync = verCfg.getSyncManager().get(event.getSession());
+            AuditProcess auditProcess = verCfg.getSyncManager().get(event.getSession());
 
             String ownerEntityName = ((AbstractCollectionPersister) collectionEntry.getLoadedPersister()).getOwnerEntityName();
             String referencingPropertyName = collectionEntry.getRole().substring(ownerEntityName.length() + 1);
@@ -266,20 +266,20 @@ public class AuditEventListener implements PostInsertEventListener, PostUpdateEv
             // null in case of collections of non-entities.
             RelationDescription rd = verCfg.getEntCfg().get(entityName).getRelationDescription(referencingPropertyName);
             if (rd != null && rd.getMappedByPropertyName() != null) {
-                generateFakeBidirecationalRelationWorkUnits(verSync, newColl, oldColl, entityName,
+                generateFakeBidirecationalRelationWorkUnits(auditProcess, newColl, oldColl, entityName,
                         referencingPropertyName, event, rd);
             } else {
                 PersistentCollectionChangeWorkUnit workUnit = new PersistentCollectionChangeWorkUnit(event.getSession(),
                         entityName, verCfg, newColl, collectionEntry, oldColl, event.getAffectedOwnerIdOrNull(),
                         referencingPropertyName);
-                verSync.addWorkUnit(workUnit);
+                auditProcess.addWorkUnit(workUnit);
 
                 if (workUnit.containsWork()) {
                     // There are some changes: a revision needs also be generated for the collection owner
-                    verSync.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), event.getAffectedOwnerEntityName(),
+                    auditProcess.addWorkUnit(new CollectionChangeWorkUnit(event.getSession(), event.getAffectedOwnerEntityName(),
                             verCfg, event.getAffectedOwnerIdOrNull(), event.getAffectedOwnerOrNull()));
 
-                    generateBidirectionalCollectionChangeWorkUnits(verSync, event, workUnit, rd);
+                    generateBidirectionalCollectionChangeWorkUnits(auditProcess, event, workUnit, rd);
                 }
             }
         }

envers/src/main/java/org/hibernate/envers/reader/AuditReaderImpl.java

@@ -35,7 +35,8 @@ import org.hibernate.envers.query.AuditEntity;
 import org.hibernate.envers.query.AuditQueryCreator;
 import static org.hibernate.envers.tools.ArgumentsTools.checkNotNull;
 import static org.hibernate.envers.tools.ArgumentsTools.checkPositive;
-import org.hibernate.envers.synchronization.AuditSync;
+
+import org.hibernate.envers.synchronization.AuditProcess;
 
 import org.hibernate.NonUniqueResultException;
 import org.hibernate.Query;
@@ -201,10 +202,10 @@ public class AuditReaderImpl implements AuditReaderImplementor {
 		}
 
 		// Obtaining the current audit sync
-		AuditSync auditSync = verCfg.getSyncManager().get((EventSource) session);
+		AuditProcess auditProcess = verCfg.getSyncManager().get((EventSource) session);
 
 		// And getting the current revision data
-		return (T) auditSync.getCurrentRevisionData(session, persist);
+		return (T) auditProcess.getCurrentRevisionData(session, persist);
 	}
 
 	public AuditQueryCreator createQuery() {

envers/src/main/java/org/hibernate/envers/synchronization/AuditProcess.java

@@ -27,38 +27,31 @@ import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.Map;
 import java.util.Queue;
-import javax.transaction.Synchronization;
 
+import org.hibernate.action.BeforeTransactionCompletionProcess;
+import org.hibernate.engine.SessionImplementor;
 import org.hibernate.envers.revisioninfo.RevisionInfoGenerator;
 import org.hibernate.envers.synchronization.work.AuditWorkUnit;
 import org.hibernate.envers.tools.Pair;
 
 import org.hibernate.FlushMode;
 import org.hibernate.Session;
-import org.hibernate.Transaction;
-import org.hibernate.event.EventSource;
 
 /**
  * @author Adam Warski (adam at warski dot org)
  */
-public class AuditSync implements Synchronization {
+public class AuditProcess implements BeforeTransactionCompletionProcess {
     private final RevisionInfoGenerator revisionInfoGenerator;
-    private final AuditSyncManager manager;
-    private final EventSource session;
 
-    private final Transaction transaction;
     private final LinkedList<AuditWorkUnit> workUnits;
     private final Queue<AuditWorkUnit> undoQueue;
     private final Map<Pair<String, Object>, AuditWorkUnit> usedIds;
 
     private Object revisionData;
 
-    public AuditSync(AuditSyncManager manager, EventSource session, RevisionInfoGenerator revisionInfoGenerator) {
+    public AuditProcess(RevisionInfoGenerator revisionInfoGenerator) {
-        this.manager = manager;
-        this.session = session;
         this.revisionInfoGenerator = revisionInfoGenerator;
 
-        transaction = session.getTransaction();
         workUnits = new LinkedList<AuditWorkUnit>();
         undoQueue = new LinkedList<AuditWorkUnit>();
         usedIds = new HashMap<Pair<String, Object>, AuditWorkUnit>();
@@ -134,47 +127,30 @@ public class AuditSync implements Synchronization {
 		return revisionData;
 	}
 
-    public void beforeCompletion() {
+    public void doBeforeTransactionCompletion(SessionImplementor session) {
         if (workUnits.size() == 0 && undoQueue.size() == 0) {
             return;
         }
 
-		try {
+        // see: http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4178431
-			// see: http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4178431
+        if (FlushMode.isManualFlushMode(session.getFlushMode())) {
-			if (FlushMode.isManualFlushMode(session.getFlushMode()) || session.isClosed()) {
+            Session temporarySession = null;
-				Session temporarySession = null;
-				try {
-					temporarySession = session.getFactory().openTemporarySession();
-
-					executeInSession(temporarySession);
-
-					temporarySession.flush();
-				} finally {
-					if (temporarySession != null) {
-						temporarySession.close();
-					}
-				}
-			} else {
-				executeInSession(session);
-
-				// Explicity flushing the session, as the auto-flush may have already happened.
-				session.flush();
-			}
-		} catch (RuntimeException e) {
-			// Rolling back the transaction in case of any exceptions
-			//noinspection finally
             try {
-                if (session.getTransaction().isActive()) {
+                temporarySession = session.getFactory().openTemporarySession();
-    			    session.getTransaction().rollback();
-                }
-            } finally {
-                //noinspection ThrowFromFinallyBlock
-                throw e;
-            }
-		}
-    }
 
-    public void afterCompletion(int i) {
+                executeInSession(temporarySession);
-        manager.remove(transaction);
+
+                temporarySession.flush();
+            } finally {
+                if (temporarySession != null) {
+                    temporarySession.close();
+                }
+            }
+        } else {
+            executeInSession((Session) session);
+
+            // Explicity flushing the session, as the auto-flush may have already happened.
+            session.flush();
+        }
     }
 }

envers/src/main/java/org/hibernate/envers/synchronization/AuditProcessManager.java

@@ -26,6 +26,8 @@ package org.hibernate.envers.synchronization;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.hibernate.action.AfterTransactionCompletionProcess;
+import org.hibernate.engine.SessionImplementor;
 import org.hibernate.envers.revisioninfo.RevisionInfoGenerator;
 
 import org.hibernate.Transaction;
@@ -34,32 +36,33 @@ import org.hibernate.event.EventSource;
 /**
  * @author Adam Warski (adam at warski dot org)
  */
-public class AuditSyncManager {
+public class AuditProcessManager {
-    private final Map<Transaction, AuditSync> auditSyncs;
+    private final Map<Transaction, AuditProcess> auditProcesses;
     private final RevisionInfoGenerator revisionInfoGenerator;
 
-    public AuditSyncManager(RevisionInfoGenerator revisionInfoGenerator) {
+    public AuditProcessManager(RevisionInfoGenerator revisionInfoGenerator) {
-        auditSyncs = new ConcurrentHashMap<Transaction, AuditSync>();
+        auditProcesses = new ConcurrentHashMap<Transaction, AuditProcess>();
 
         this.revisionInfoGenerator = revisionInfoGenerator;
     }
 
-    public AuditSync get(EventSource session) {
+    public AuditProcess get(EventSource session) {
-        Transaction transaction = session.getTransaction();
+        final Transaction transaction = session.getTransaction();
-
+        
-        AuditSync verSync = auditSyncs.get(transaction);
+        AuditProcess auditProcess = auditProcesses.get(transaction);
-        if (verSync == null) {
+        if (auditProcess == null) {
             // No worries about registering a transaction twice - a transaction is single thread
-            verSync = new AuditSync(this, session, revisionInfoGenerator);
+            auditProcess = new AuditProcess(revisionInfoGenerator);
-            auditSyncs.put(transaction, verSync);
+            auditProcesses.put(transaction, auditProcess);
 
-            transaction.registerSynchronization(verSync);
+            session.getActionQueue().registerProcess(auditProcess);
+            session.getActionQueue().registerProcess(new AfterTransactionCompletionProcess() {
+                public void doAfterTransactionCompletion(boolean success, SessionImplementor session) {
+                    auditProcesses.remove(transaction);
+                }
+            });
         }
 
-        return verSync;
+        return auditProcess;
-    }
-
-    public void remove(Transaction transaction) {
-        auditSyncs.remove(transaction);
     }
 }

envers/src/test/java/org/hibernate/envers/test/integration/reventity/ExceptionListener.java

@@ -40,7 +40,7 @@ public class ExceptionListener extends AbstractEntityTest {
         cfg.addAnnotatedClass(ExceptionListenerRevEntity.class);
     }
 
-    @Test
+    @Test(expectedExceptions = RuntimeException.class)
     public void testTransactionRollback() throws InterruptedException {
         // Trying to persist an entity - however the listener should throw an exception, so the entity
 		// shouldn't be persisted
@@ -49,9 +49,12 @@ public class ExceptionListener extends AbstractEntityTest {
         StrTestEntity te = new StrTestEntity("x");
         em.persist(te);
         em.getTransaction().commit();
+    }
 
+    @Test(dependsOnMethods = "testTransactionRollback")
+    public void testDataNotPersisted() {
 		// Checking if the entity became persisted
-		em = getEntityManager();
+		EntityManager em = getEntityManager();
         em.getTransaction().begin();
         Long count = (Long) em.createQuery("select count(s) from StrTestEntity s where s.str = 'x'").getSingleResult();
 		assert count == 0l;