Re: [infinispan-dev] AtomicHashMap concurrent modifications in pessimistic mode

On Mon, May 20, 2013 at 1:57 PM, Manik Surtani <msurtani(a)redhat.com&gt; wrote: > > On 16 May 2013, at 15:04, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> Hi guys >> >> I'm working on an intermittent failure in NodeMoveAPIPessimisticTest and I think I've come across what I think is underspecified behaviour in AtomicHashMap. >> >> Say we have two transactions, tx1 and tx2, and they both work with the same atomic map in a pessimistic cache: >> >> 1. tx1: am1 = AtomicMapLookup.get(cache, key) >> 2. tx2: am2 = AtomicMapLookup.get(cache, key) >> 3. tx1: am1.put(subkey1, value1) // locks the map >> 4. tx2: am2.get(subkey1) // returns null >> 5. tx1: commit // the map is now {subkey1=value1} >> 6. tx2: am2.put(subkey2, value2) // locks the map >> 7. tx2: commit // the map is now {subkey2=value2} >> >> It's not clear to me from the AtomicMap/AtomicHashMap javadoc if this is ok or if it's a bug... > > If optimistic, step 7 should fail with a write skew check. If pessimistic, step 2 would *usually* block assuming that another thread is updating the map, but since neither tx1 or tx2 has started updating the map yet, neither has a write lock on the map. So that succeeds. I'm not sure if this is any different from not using an atomic map: > > 1. tx1: cache.get(k, v); // reads into tx context > 2. tx2: cache.get(k, v); > 3. tx1: cache.put(k, v + 1 ); > 4. tx1: commit > 5. tx2: cache.put(k, v + 1 ); > 6. tx2: commit > > here as well, if using optimistic, step 6 will fail with a WSC but if pessimistic this will work (since tx2 only requested a write lock after tx1 committed/released its write lock). The difference is that in your scenario, you see in the code that tx2 writes to key k, so it's not surprising to find that tx2 overwrote the value written by tx1. But it would be surprising if tx2 also overwrote an unrelated key k2. With an atomic map, you only see in the code "map.put(subkey2, value)". Tx2 doesn't touch subkey1, so it's not that obvious that it should remove it. It is clear to me why it behaves the way it does now, after reading the implementation, but I don't think it's what most users would expect. (The proof, I guess, is in the current implementation of TreeCache.move()).

With a FineGrainedAtomicMap in optimistic mode, it's not obvious why tx1 writing to subkey1 should cause tx2's write to subkey2 fail, either (see https://issues.jboss.org/browse/ISPN-3123).

>> Note that today the map is overwritten by tx2 even without step 4 ("tx2: am2.get(subkey1)"). I'm pretty sure that's a bug and I fixed it locally by using the FORCE_WRITE_LOCK in AtomicHashMapProxy.getDeltaMapForWrite. >> >> However, when the Tree API moves a node it first checks for the existence of the destination node, which means NodeMoveAPIPessimisticTest is still failing. I'm not sure if I should fix that by forcing a write lock for all AtomicHashMap reads, for all TreeCache reads, or only in TreeCache.move(). > > I think only in TreeCache.move() I tend to disagree. I think it's way too easy to introduce a read of a node's structure in a transaction and start losing data without knowing it. _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev