[hibernate-dev] ReadWrite 2LC "read uncommitted" data anomaly
Radim Vansa
rvansa at redhat.com
Wed Apr 6 07:02:39 EDT 2016
Hi Vlad,
I think that this change is safe (it uses only the supported flow of 2LC
SPI calls), though it introduces a bit more overhead for the refresh.
FYI, besides unit tests, there's CorrectnessTestCase [1] - I've used
this one to find out most of flaws in Infinispan 2LC impl. Could be
worth a try running this on EHCache, too.
I've noticed some of the added tests explicitly use H2. Hibernate uses
1.3 for the testsuite - I was running tests with both 1.3 and 1.4 and
the behaviour can differ a lot, since 1.4 can lock separate rows and 1.3
locks just whole tables (pardon me if I recall that incorrectly) - this
can lead to different timing, and eventually to deadlock/test failure.
Just to let you know that I had problems with these.
Radim
[1]
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/stress/CorrectnessTestCase.java
On 04/06/2016 12:17 PM, Vlad Mihalcea wrote:
> Hi Radim,
>
> I pushed this fix on master and 5.1, and I managed to add an EHCache
> where the same behavior was replicated as well:
>
> https://github.com/hibernate/hibernate-orm/commit/cbdab9d87f05b4255c7930a32fe995f87f0f3e0b
>
> For Infinispan, I think it's better if you can investigate if this is
> an issue or if the change does not break anything either (all
> Infinispan tests ran fine, so hopefully this should not be the case).
>
> Thanks,
> Vlad
>
> On Wed, Apr 6, 2016 at 12:58 PM, Radim Vansa <rvansa at redhat.com
> <mailto:rvansa at redhat.com>> wrote:
>
> On 04/05/2016 04:13 PM, Vlad Mihalcea 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.
>
> This is a different matter; in Infinispan 2LC impl you store locks and
> entries either in two different caches (the entries cache is
> invalidation, locks is local), or in single cache
> (replicated/distributed). As we don't want to lose locks randomly, and
> eviction picks entries unpredictably, its use is discouraged.
>
> I think that this issue does not apply to Infinispan with the
> invalidation configuration, since evict/evictAll does not remove any
> locks, and the lock blocks further updates (including putFromLoads) to
> the entry in cache until the transaction commits. In case of
> replicated/distributed cache, it seems that the evict is ignored after
> update, but evictAll is not (that would qualify as a bug) - so after
> evictAll you could observe the uncommitted read. Nevertheless I would
> have to test this.
>
> Radim
>
> >
> > 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 <mailto:sanne at hibernate.org>> wrote:
> >
> >> On 5 April 2016 at 14:11, Vlad Mihalcea
> <mihalcea.vlad at gmail.com <mailto: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
> <mailto:hibernate-dev at lists.jboss.org>
> >>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
> > _______________________________________________
> > hibernate-dev mailing list
> > hibernate-dev at lists.jboss.org <mailto:hibernate-dev at lists.jboss.org>
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
> --
> Radim Vansa <rvansa at redhat.com <mailto:rvansa at redhat.com>>
> JBoss Performance Team
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org <mailto:hibernate-dev at lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
--
Radim Vansa <rvansa at redhat.com>
JBoss Performance Team
More information about the hibernate-dev
mailing list