[infinispan-dev] Dangers of getCacheEntry (ISPN-4424)

[Galder Zamarreño]

Pedro’s suggestion looks good [1]

It’s passed tests both locally (20 threads, each sending 200 operations) and in a remote server (40 threads, each sending 1000 operations).

Cheers,

[1] https://github.com/infinispan/infinispan/pull/2671

On 25 Jun 2014, at 10:21, Galder Zamarreño <galder at redhat.com> wrote:

> 
> On 24 Jun 2014, at 19:51, Pedro Ruivo <pedro at infinispan.org> wrote:
> 
>> 
>> 
>> On 06/24/2014 05:11 PM, Mircea Markus wrote:
>>> 
>>> On Jun 24, 2014, at 16:50, Galder Zamarreño <galder at redhat.com> wrote:
>>> 
>>>> 
>>>> On 24 Jun 2014, at 16:51, Mircea Markus <mmarkus at redhat.com> wrote:
>>>> 
>>>>> 
>>>>> On Jun 24, 2014, at 15:27, Galder Zamarreño <galder at redhat.com> wrote:
>>>>> 
>>>>>> To fix this, I’ve been working with Dan on some solutions and we’ve taken inspiration of the new requirements appearing as a result of ISPN-2956. To be able to deal with partial application of conditional operations properly, transactional caches are needed. So, the solution that can be seen in [4] takes that, and creates a transaction around the replaceIfUmodified and forces the getCacheEntry() call to acquire lock via FORCE_WRITE_LOCK flag.
>>>>> 
>>>>> so the entire underlaying cache needs to be transactional then?
>>>> 
>>>> Yeah, it needs to be transactional, but the code I’ve written also deals with the fact that the cache might not be transactional. I’ll probably add a WARN message when it’s not transactional. This goes in hand with the recommendations for ISPN-2956, whereby failover for conditional operations relying on return values require transactional caches to properly deal with failover situations.
>>>> 
>>>> To sum up, if using conditional operations or CRUD methods with Flag.FORCE_RETURN_VALUE, caches should be transactional. Moreover, to achieve concurrency guarantees of counter tests such as the one tested for 4424, locking needs to be pessimistic too. If not using conditional operations or CRUD methods without the flag, the cache could be non-transactional.
>>> 
>>> Thanks for the analysis. I think we should go with your patch for ISPN 7.0 and consider the proper solution for the future, as you suggest below.
>>> +1 for the warning, users should be made aware for the limitation.
>> 
>> +1 for the patch.
>> 
>> Another suggestion: in *InternalEntryFactory.update()*, you can 
>> synchronize in the cache entry, and create a new method 
>> *InternalCacheEntry copy(InternalCacheEntry)* that makes a copy while it 
>> also synchronizes in the existing cache entry. I think in this way, you 
>> don't need the cache to be transactional neither to force the lock on 
>> reads. Also, I would suggest the copy() to be invoked in your case (or 
>> in the conditional commands accesses to DataContainer?).
> 
> ^ This is an interesting idea too. While transactions are necessary to combat ISPN-2956, fixing this via transactions, as you all pointed out, might be a bit too much and also might hurt performance wise.
> 
> I’m doing further testing to see if I see Takayoshi’s problems locally. In the mean time, I’ll try to prototype an alternative solution based around this.
> 
>> 
>>>> 
>>>>> 
>>>>>> This solves the issue explained above, but of course it has an impact of the performance. The test now runs about 1.5 or 2 times slower.
>>>>>> 
>>>>>> This is probably the best that we can do in the 7.0 time frame, but there’s several things that could improve this:
>>>>>> 
>>>>>> 1. True immutable entries in the cache. If the entries in the cache were truly immutable, there would be no risk of sending back a partially correct entry back to the client.
>>>>>> 
>>>>>> 2. A cache replace method that does not compare objects based on equality (the current replace()), but a replace method that takes a function. The function could compare that the old entry’s version and the cached entry’s version match. The function would only be executed inside the inner container, with all the locks held properly. I already hinted something similar in [5].
>>> 
>>> 
>>> Shall we raise a new JIRA for the permanent solution then?
>> 
>> +1
>> 
>> I think the immutable entries would be better, but it will destroy 
>> ReadCommitted isolation. The ReadCommitted is based on the mutability of 
>> the entry (i.e. it will not invoke the data container if the entry 
>> exists in transaction context). When (and if) is removed, then we can 
>> make the entries immutable.
> 
> Yeah, I think RC is the biggest barrier right now. 
> 
> Thanks everyone for their input so far!! :D
> 
> Cheers,
> --
> Galder Zamarreño
> galder at redhat.com
> twitter.com/galderz
> 
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Galder Zamarreño
galder at redhat.com
twitter.com/galderz