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

[Radim Vansa]

On 04/06/2016 11:58 AM, Radim Vansa 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.

... it's use is discouraged in the repl/dist mode as you can't allow 
eviction on entries but prohibit on locks with both of these in single 
cache.

>
> 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> 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
>


-- 
Radim Vansa <rvansa at redhat.com>
JBoss Performance Team