Delayed operation optimization in PersistentBag and PersistentList can corrupt 2nd level cache

[eugene75]

The delayed operation optimization in PersistentBag and PersistentList can corrupt the second level cache under specific conditions.

Assume we two entities Parent and Child with a bi-directional one-to-many from Parent to Child with 2nd level caching enabled for both entities and both sides of the relationship and at the beginning of the transaction the 2nd level cache is empty.  The transaction flow as follows will result in a corrupt 2nd level cache:

// add a new Child to Parent

//  the add operation is added to the delayed queue and the collection is not initialized or marked as dirty

Child newChild = new Child();

newChild.setParent(parent);

parent.getChildren().add(newChild);

// do something that causes an entity manager flush, either explicit call or a query

//  this causes newChild to be written to the database

entityManager.flush();

// remove an existing child from parent

//   the remove() call will cause the collection to get initialized from the database, which includes the newly added child

//   because the collection does not think it is dirty, it sticks the relationship data in the 2nd level cache

Child oldChild = parent.findChild("name");

oldChild.setParent(null);

parent.getChildren().remove(oldChild);

entityManager.remove(oldChild);

// at this point if the transaction fails, the 2nd level cache is not cleaned up and the Parent->Child relationship contains an ID that does not exist in the database

//  even if the transaction eventually succeeds, there is a period of time when the 2nd level cache contains uncommitted data not visible to any other transaction

The code in AbstractPersistentCollection.afterInitialize() that controls whether or not a collection is added to the 2nd level cache does check the delayed operation queue and block the 2nd level cache put if it is non-empty.  But it does account for the fact that a previous flush() within the same transaction may have cleared the delayed operation queue.  It seems to me that once a collection has been modified in the course of a transaction, it should  veto the 2nd level cache put for the duration of that transaction.

Our workaround is to provide our own UserCollectionType that forgos use of the delayed operation queue or overrides the endRead() method to veto caching.

[eugene75]

We decided instead to modify the conditions under which a collection is placed in the 2nd level cache during loading.  Our patch to Hibernate 4.1.7 is below.

diff --git a/hibernate-core/src/main/java/org/hibernate/collection/internal/AbstractPersistentCollection.java b/hibernate-core/src/main/java/org/hibernate/collection/internal/AbstractPersistentCollection.java
index 80db62e..d90ab51 100644
--- a/hibernate-core/src/main/java/org/hibernate/collection/internal/AbstractPersistentCollection.java
+++ b/hibernate-core/src/main/java/org/hibernate/collection/internal/AbstractPersistentCollection.java
@@ -65,6 +65,8 @@ import org.hibernate.type.Type;
 public abstract class AbstractPersistentCollection implements Serializable, PersistentCollection {
      private static final Logger log = Logger.getLogger( AbstractPersistentCollection.class );
 
+     private static final boolean useModifiedFlag = !Boolean.getBoolean("geneinsight.hibernate.disable-modified-flag");
+     
      private transient SessionImplementor session;
      private boolean initialized;
      private transient List operationQueue;
@@ -78,6 +80,10 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
      // collections detect changes made via their public interface and mark
      // themselves as dirty as a performance optimization
      private boolean dirty;
+     // track if any change happens to this collection
+     // set when dirty is set, but wont be cleared 
+     private boolean modified = false;
+     
      private Serializable storedSnapshot;
 
      private String sessionFactoryUuid;
@@ -105,8 +111,17 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
 
      public final void dirty() {
           dirty = true;
+          modified = true;
      }
 
+     public final boolean isModified() {
+         if (useModifiedFlag) {
+             return modified;
+         } else {
+             return false; 
+         }  
+     }
+     
      public final Serializable getStoredSnapshot() {
           return storedSnapshot;
      }
@@ -424,7 +439,7 @@ public abstract class AbstractPersistentCollection implements Serializable, Pers
                operationQueue = new ArrayList( 10 );
           }
           operationQueue.add( operation );
-          dirty = true; //needed so that we remove this collection from the second-level cache
+          dirty(); //needed so that we remove this collection from the second-level cache
      }
 
      /**
diff --git a/hibernate-core/src/main/java/org/hibernate/engine/loading/internal/CollectionLoadContext.java b/hibernate-core/src/main/java/org/hibernate/engine/loading/internal/CollectionLoadContext.java
index 72a5e68..83596f2 100644
--- a/hibernate-core/src/main/java/org/hibernate/engine/loading/internal/CollectionLoadContext.java
+++ b/hibernate-core/src/main/java/org/hibernate/engine/loading/internal/CollectionLoadContext.java
@@ -38,6 +38,7 @@ import org.hibernate.EntityMode;
 import org.hibernate.HibernateException;
 import org.hibernate.cache.spi.CacheKey;
 import org.hibernate.cache.spi.entry.CollectionCacheEntry;
+import org.hibernate.collection.internal.AbstractPersistentCollection;
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.engine.spi.CollectionEntry;
 import org.hibernate.engine.spi.CollectionKey;
@@ -258,7 +259,15 @@ public class CollectionLoadContext {
                     persister.hasCache() &&             // and the role has a cache
                     session.getCacheMode().isPutEnabled() &&
                     !ce.isDoremove();                   // and this is not a forced initialization during flush
-          if ( addToCache ) {
+          
+          boolean isModified = false;
+          PersistentCollection pc = lce.getCollection();
+          if (pc instanceof AbstractPersistentCollection)
+          {
+              isModified = ((AbstractPersistentCollection)pc).isModified();
+          }
+          
+          if ( addToCache && !isModified) {
                addCollectionToCache( lce, persister );
           }
 


 

[smarlow]

I don't think that the Hibernate team is monitoring this forum, please try using the Hibernate forum instead (see http://hibernate.org/ for forum link and mailing list info).  You can link to this message if you like.  If you create a Hibernate jira for this issue, please attach your patch to that (should include a unit test similar to the other second level cache tests).

Scott