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

[Steve Ebersole]

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 at 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 <sanne at hibernate.org>
> wrote:
>
> > On 5 April 2016 at 14:11, Vlad Mihalcea <mihalcea.vlad at 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 at lists.jboss.org
> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev