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

[Galder Zamarreño]

FYI: Takayoshi has run some further testing and found that with 40 threads there was still some deviation, so it needs further investigation.

On 24 Jun 2014, at 16:27, Galder Zamarreño <galder at redhat.com> wrote:

> Hi all,
> 
> Last few days I’ve been having fun fixing [1] and the solution I’ve got to has some links to [2].
> 
> The problem is that when getCacheEntry() is called, the entry returned could be partially correct. Up until now, we’d send back a cache entry back to the client that the user could not modify, but it was linked to the data container cache entry, which is mutable and hence could change on the fly. As a result of this, the value could sometimes be updated but the version would belong to a previous value for example.
> 
> A first attempt to fix it was to provide a true immutable cache entry view which was initialised on construction, but even here there was the chance of having value and version mistmatch, because getCacheEntry does not acquire locks, so an ongoing update could result in the cache entry being constructed when the update was half way, so, it would have the right value but the old version information.
> 
> All this didn’t work well with the replaceIfUmodified logic in [3]. For example, you could easily get this situation:
> 
> Current Cache contents: key=k1, value=A, version=1
> 
> T1: Hot Rod client calls: replace(k1, value=B, old-version=1)
> T2: Hot Rod client calls: replace(k1, value=B, old-version=1)
> T1: Server calls getCacheEntry and retrieves value=A,version=1
> T1: Cached and stream versions match (both are 1), so call replace(k1, old-value=A, new-value=B)
> T1: Server updates value to B but version is still 1
> T2: Server calls getCacheEntry and retrieves value=B,version=1 (wrong!)
> T1: Cached and stream versions match (both are 1), so call replace(k1, old-value=B, new-value=B)
> T1: Server updates version to 2 
> T1: Returns Success, replace worked
> T2: Returns Success, replace worked
> 
> The end result is that cache contained B, but both replaces returned true. This is wrong and would fail to consistenly keep a counter in the value part. Imagine the value being a counter of number of times the replace succeeded. In the test developed by Takayoshi, N times replace() would return true, but the final value would be N-1 or N-2, or N-5 :|
> 
> 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. 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].
> 
> Finally, this was not a problem when the value stored in Hot Rod was a ByteArrayValue wrapping the byte array and the version, because the value was treated atomically, and in hindsight, maybe adding getCacheEntry might have been premature, but this method has proven useful for other use cases too (rolling upgrades…etc).
> 
> Cheers,
> 
> [1] https://issues.jboss.org/browse/ISPN-4424
> [2] https://issues.jboss.org/browse/ISPN-2956
> [3] https://github.com/infinispan/infinispan/blob/master/server/core/src/main/scala/org/infinispan/server/core/AbstractProtocolDecoder.scala#L302
> [4] https://github.com/galderz/infinispan/tree/t_4424
> [5] https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/AdvancedCache.java#L374
> --
> 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