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

[Vlad Mihalcea]

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 <steve at 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.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
>>
>