That's exactly what I mean too. I think we can improve it a little bit when the evict is forced while being on running Session. But that's not in the scope of the original issue which stays as is.
I'll add a new Jira issue with a test case, and a design change proposal that we can discuss when we have some spare time. Vlad On Tue, Apr 5, 2016 at 10:00 PM, Steve Ebersole <st...@hibernate.org> wrote: > 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.v...@gmail.com> > 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-write >> > >> >> 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 <sa...@hibernate.org> >> wrote: >> >> > On 5 April 2016 at 14:11, Vlad Mihalcea <mihalcea.v...@gmail.com> >> 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-cacheconcurrencystrategy-work/ >> > > >> > > 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@lists.jboss.org >> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >> _______________________________________________ >> hibernate-dev mailing list >> hibernate-dev@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
