Re: [hibernate-dev] ReadWrite 2LC "read uncommitted" data anomaly

The contract for SessionFactory-level even says that it operates outside of the confines of an Session-based locking of the cached data. So it works this way by design. We can question the design decision, but that's a different discussion. On Tue, Apr 5, 2016 at 9:38 AM Vlad Mihalcea <mihalcea.vlad(a)gmail.com&gt; wrote: > Hi, > > I'd definitely fix it for the refresh operation, which does an implicit > cache eviction too. > In this case, the proposed PR solution is fine since it simply locks the > entry right after it is evicted from the cache and releases the lock after > the transaction is ended. > This way, we won't push an uncommitted entity into 2PL during the > two-phase > loading phase that is triggered by the refresh operation. > > For sessionFactory.getCache.evictEntity/evictCollection, if there is a > current Session going on, we could propagate a > CacheEvictEvent/CollectionCacheEvictEvent which can apply the lock on that > particular entity/collection, and we release it right after the current > transaction is ended, similar to what refresh should do as well. > > For every other use case, like evictAll/evictRegion, we should just > document the behavior. > > I saw that Radim has added such a warning for Infinispan in the new User > Guide: > > read-write mode is supported on non-transactional distributed/replicated > > caches, however, eviction should not be used in this configuration. Use > of > > eviction can lead to consistency issues. > > > Unfortunately, the EhCache documentation > < > http://www.ehcache.org/documentation/2.8/integrations/hibernate.html#read... > > > > makes a wrong assumption: > > read-write - Caches data that is sometimes updated while maintaining the > > semantics of “read committed” isolation level. > > > Vlad > > On Tue, Apr 5, 2016 at 4:30 PM, Sanne Grinovero <sanne(a)hibernate.org&gt; > wrote: > > > On 5 April 2016 at 14:11, Vlad Mihalcea <mihalcea.vlad(a)gmail.com&gt; > wrote: > > > Hi, > > > > > > While reviewing the PR for this issue: > > > > > > https://hibernate.atlassian.net/browse/HHH-10649 > > > > > > I realized that the ReadWrite cache concurrency strategy has a flaw > that > > > permits "read uncommitted" anomalies. > > > The RW cache concurrency strategy guards any modifications with Lock > > > entries, as explained in this post that I wrote some time ago: > > > > > > > > > http://vladmihalcea.com/2015/05/25/how-does-hibernate-read_write-cachecon... > > > > > > Every time we update/delete an entry, a Lock is put in the cache under > > the > > > entity key, and, this way, "read uncommitted" anomalies should be > > prevented. > > > > > > The problem comes when entries are evicted either explicitly: > > > > > > session.getSessionFactory().getCache().evictEntity( > CacheableItem.class, > > > item1.getId() ); > > > > > > or implicitly: > > > > > > session.refresh( item1 ); > > > > Good catch! > > > > I think this is caused as we generally don't expect the evict > > operation to be controlled explicitly. > > In my personal experience, I would use the evictAll method to nuke the > > cache state after some significant operation, like restoring a > > backup.. and no other Session would have been opened in the meantime. > > I never used an explicit single-shot evict so I can't say what the use > > case would be. > > > > But of course you're right that it might be used differently, or at > > least such a limitation should be clear. > > > > > > > > During eviction, the 2PL will remove the Lock entry, and if the user > > > attempts to load the entity anew (in the same transaction that has > > modified > > > the entity but which is not committed yet), an uncommitted change > could > > be > > > propagated to the 2PL. > > > > > > This issue is replicated by the PR associated to this Jira issue, and > I > > > also replicated it with manual eviction and entity loading. > > > > > > To fix it, the RW cache concurrency strategy should not delete entries > > from > > > 2PL upon eviction, but instead it should turn them in Lock entries. > > > > I'm not sure I understood this part. Shouldn't it rather be allowed to > > delete everything, except any existing locks? > > Then rather than turn the remaining locks into locks, it would be > > enough to leave them. > > > > > For the evict method, this is not really a problem, but evictAll would > > > imply taking all entries and replacing them with Locks, and that might > > not > > > perform very well in a distributed-cache scenario. > > > > > > Ideally, lock entries would be stored separately than actual cached > value > > > entries, and this problem would be fixed in a much cleaner fashion. > > > > I'd leave this as a detail to the Cache implementation, some might be > > able to perform some operation more efficiently. > > Probably a good idea to clarify this expectation on the javadocs of > > the SPI methods. > > > > Thanks, > > Sanne > > > > > > > > > > Let me know what you think about this. > > > > > > Vlad > > > _______________________________________________ > > > hibernate-dev mailing list > > > hibernate-dev(a)lists.jboss.org > > > https://lists.jboss.org/mailman/listinfo/hibernate-dev > > > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >