On Jun 24, 2014, at 15:27, Galder Zamarreño <gal...@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. so the entire underlaying cache needs to be transactional then? > 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 > gal...@redhat.com > twitter.com/galderz > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev Cheers, -- Mircea Markus Infinispan lead (www.infinispan.org) _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
